-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Dpe 135): naming rules #85
Conversation
EthanHonzikSPS
commented
Jul 10, 2024
- Created a ruleset schema
- installed jest types
- added a tsconfig.json such that JS files will be validated for type errors
- updated the camel case rule to use the built in casing function instead of regex to get performance
- Addressed all the comments on the confluence page
- Updated documentation with links
…ity, added sps-disallowed-prepositions rule
Performance:Overall performance improved from 30 seconds down to 23 seconds to run the entire performance test suite. I suspect this is due to using the built in casing function instead of regex. Problems:Camel CaseCamel case rule has a very large impact on Identity-service with multi-hundred errors. Network-configuration-service has over 1000 errors. We will likely have to change the severity from error to warning. GeneralWarnings had a large jump due to the various naming conventions network-configuration-service again had the largest jump with around 600 additional warnings from the original 254 warnings |
Signed-off-by: Ethan Honzik <[email protected]>
Signed-off-by: Ethan Honzik <[email protected]>
Out of scope for PR Signed-off-by: Ethan Honzik <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just a few copy/paste issues. As far as the impact test, we should chat as a team about whether we need to start leveraging the recommended property -> https://docs.stoplight.io/docs/spectral/0a73453054745-recommended-or-all
rulesets/test/naming/sps-mandate-abbreviations-identifier.test.js
Outdated
Show resolved
Hide resolved
rulesets/test/naming/sps-mandate-abbreviations-organization.test.js
Outdated
Show resolved
Hide resolved
rulesets/test/naming/sps-mandate-abbreviations-organization.test.js
Outdated
Show resolved
Hide resolved
rulesets/test/naming/sps-mandate-abbreviations-reference.test.js
Outdated
Show resolved
Hide resolved
rulesets/test/naming/sps-mandate-abbreviations-reference.test.js
Outdated
Show resolved
Hide resolved
rulesets/test/naming/sps-mandate-abbreviations-reference.test.js
Outdated
Show resolved
Hide resolved
rulesets/test/naming/sps-mandate-abbreviations-organization.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the impact specifically on network API related specs (ones that should be conforming already)?
What was the original rule you were referring to with the regex for camelCase? I didn't see it in the comparison of |
Removed accidentally committed tsconfig.json, out of scope for this PR Signed-off-by: Ethan Honzik <[email protected]>
…/sps-api-standards into DPE-135-naming-rules
With camel case changed to a warning there's a couple additional errors that occur with the sps-invalid-created-date-time-type rule. Additionally there is a bit of uptick in warnings anywhere from 1 to 9. Most of these are camel case issues but there's a couple miscellaneous ones as well.
sps-camel-case-properties regex when I first cloned it, This I'm now realizing was included in @brandonsahadeo's first commit to this branch and wasn't a part of main. |
OK as long as we feel confident that these are legit issues detected and are mistakes by the team then this is great. How many warnings do the camel case change generate on the network apis? Is it a lot?
ah OK makes more sense, was looking for it. So no real performance change against main then. |
|
# [1.12.0](v1.11.4...v1.12.0) (2024-07-17) ### Features * **Dpe 135:** naming rules ([#85](#85)) ([cd1301c](cd1301c))