CLOUDP-306582: IPA-126: Top-Level API Names #668
Conversation
| if (!tagName || tagName.trim().length === 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If we don't want to error on non-existent names, I think you can remove this check and in the rule do:
given: $.tags[*].name
There was a problem hiding this comment.
$.tags[*].name this gives the name as input, but for exception level, I wanted to keep it at tag object level. Anyhow I changed the given to $.tags[?(@.name && @.name.length > 0)] to get the ones with non-empty name
| { | ||
| code: 'xgen-IPA-126-tag-names-should-use-title-case', | ||
| message: 'Tag name should use Title Case, found: "Push-Based Log Export".', | ||
| path: ['tags', '2'], | ||
| severity: DiagnosticSeverity.Warning, | ||
| }, |
There was a problem hiding this comment.
I wonder if we should allow dashes since Push-Based or Third-Party should be valid IMO
There was a problem hiding this comment.
We can say that if the second word is capitalized in a hyphenated word, it is valid. This is your suggestion, am I correct?
There was a problem hiding this comment.
Added that support, and updated the unit tests accordingly
| { | ||
| code: 'xgen-IPA-126-tag-names-should-use-title-case', | ||
| message: 'Tag name should use Title Case, found: "AWS Clusters DNS".', | ||
| path: ['tags', '3'], | ||
| severity: DiagnosticSeverity.Warning, | ||
| }, |
There was a problem hiding this comment.
I think we should allow capital case abbreviations/initialism as well, like API, AWS etc.
There was a problem hiding this comment.
I can construct a list of abbreviations to be ignored. Do you have any suggestions?
There was a problem hiding this comment.
Sure, that could be good, from top of mind I'd propose:
- AWS
- GCP
- IP
- API
- MongoDB
- LDAP
- DNS
There was a problem hiding this comment.
I included some ignoreList to maintain their casing, and some grammaticalWords to allow them in lowercase. Could you take a look and let me know how it sounds?
| const hyphenatedParts = wordGroup.split('-'); | ||
| return hyphenatedParts.every((part) => { | ||
| if (part === '') return true; // Skip empty parts | ||
| if (ignoreList.includes(part)) return true; |
There was a problem hiding this comment.
If there is a name like This -Name with a space beofre/after the dash, a part here could be empty, I think that should be considered invalid, WDYT?
There was a problem hiding this comment.
Yes, I agree there should be no space, let me fix it
There was a problem hiding this comment.
Fixed it, and added some test cases, let me know if I miss anything
|
|
||
| // For regular words | ||
| if (ignoreList.includes(wordGroup)) return true; | ||
| if (grammaticalWords.includes(wordGroup)) return true; |
There was a problem hiding this comment.
[Nit] These should be for words that are not the first word, e.g. The Name and User of the Thing is valid, but the User would be invalid.
| if (grammaticalWords.includes(wordGroup)) return true; | |
| if (index !== 0 && grammaticalWords.includes(wordGroup)) return true; |
There was a problem hiding this comment.
Makes sense! Applied the suggestion, and added a unit test for it
| const hyphenatedParts = wordGroup.split('-'); | ||
| return hyphenatedParts.every((part) => { | ||
| if (ignoreList.includes(part)) return true; | ||
| if (grammaticalWords.includes(part)) return true; |
There was a problem hiding this comment.
| if (grammaticalWords.includes(part)) return true; |
[nit] There are probably no compound words (I think that's what it's called 😋 ) using dashes with words like "the" or "a"
There was a problem hiding this comment.
Yes, that's a good catch! Applied the suggestion, thanks!
2 participants
Proposed changes
Jira ticket: CLOUDP-306582
Found 13 violations
Checklist
Changes to Spectral
Further comments