feat: extend the functionality of inconsistent-naming
#166
Conversation
🦋 Changeset detectedLatest commit: a9b25be The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
High-quality code as always. I have only several minor suggestions/questions. Thank you!
I highly appreciated the get-main-subject utility, it is a great example of good logic! 👍
The corresponding issue also has a subtask about auto-fixes. As I see, the auto-fixes have already been added to this rule. Can we consider that subtask as done?
| ]) | ||
| }) | ||
|
|
||
| it('allows inconsistency between different slice groups', () => { |
There was a problem hiding this comment.
Should this one also be renamed to entity instead of slice, like the other test case names?
| expect(diagnostics).toEqual([ | ||
| { | ||
| message: 'Inconsistent pluralization of slice names. Prefer all plural names', | ||
| message: 'Inconsistent pluralization of entity names. Prefer all plural names.', | ||
| fixes: [ | ||
| { | ||
| type: 'rename', |
There was a problem hiding this comment.
(non-blocking): Should it prefer singular names in this case (when it's 50/50)? It looks to me as a more favorable convention, which is more common in documentation examples, etc
There was a problem hiding this comment.
yeah, I didn't really think about it before, I agree
| @@ -82,3 +112,48 @@ it('prefers the singular form when there are more singular slices', () => { | |||
| }, | |||
| ]) | |||
| }) | |||
|
|
|||
| it('recognizes the special case when there are two pluralizations of the same name', () => { | |||
There was a problem hiding this comment.
when there are two pluralizations of the same name
It seems a little misleading, more like a typo, should it be "when there is a plural and singular form of the same name"
| const message = 'Inconsistent pluralization of entity names' | ||
| if ( | ||
| pluralNames.length >= singularNames.length && | ||
| singularNames.some(([name]) => !duplicates.singular.includes(name)) |
There was a problem hiding this comment.
Should we filter the duplicates out somewhere at the top at once? Just keep the code a bit cleaner and remove the need to filter out the duplicates in 4 places
There was a problem hiding this comment.
I'll try, but the logic here is pretty convoluted, so maybe it won't work, no promises :)
|
I marked the autofixes task as incomplete because it didn't yet support the cases where there's a singular and a plural form. It should be done completely with this PR |
Closes #8
Extends the functionality of
inconsistent-namingto support multi-word names and handle rename collisions