feat(eslint-plugin): add rule 'sort-ngmodule-metadata-arrays' for array sort order of modules #386
Conversation
There was a problem hiding this comment.
Thank you @blingerson, this seems like something that would be useful to some folks!
Rather than adding to the README, from now on we would like to ensure that new rules always provide an entry in docs/rules/.
Please could you take a look at the existing example in packages/eslint-plugin/docs/rules and add one for your rule?
|
@JamesHenry, will do, should I remove the changes from the README as well? Thanks. |
|
Yes please |
|
Done, I originally had the module class identifiers on separate lines. Perhaps prettier formats the code blocks in the .md files as well? |
Nx Cloud ReportCI ran the following commands for commit ed93f9b. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch Sent with 💌 from NxCloud. |
There was a problem hiding this comment.
Hello @blingerson, thanks for your contribution. I commented on some points that I found relevant. Please have a look.
packages/eslint-plugin/tests/rules/ngmodule-array-sorted.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/ngmodule-array-sorted.test.ts
Outdated
Show resolved
Hide resolved
blingerson
changed the title
packages/eslint-plugin/tests/rules/sort-ngmodule-arrays.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/sort-ngmodule-arrays.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/sort-ngmodule-arrays.test.ts
Outdated
Show resolved
Hide resolved
|
I sent the review again, as it gets outdated after the files were renamed. |
|
Ok, hopefully I got all the small issues and semantic stuff this time. |
blingerson
force-pushed
the
master
branch
2 times, most recently
from
a1e604a to
0fa0f43
Compare
|
Sorry for the ugly git history guys, I realize now I clicked the update with master button which complicates my ability to squash. If it is easier for me to just create a new fork with a clean set of changes in a single commit I don't mind. let me know how you would like me to proceed |
|
@rafaelss95 @JamesHenry Ok, was able to fix up the commit history without doing any harm with some git magic. Thanks for bearing with me. Next time I will read the gitflow best practices FIRST. ;) |
|
@blingerson we always squash merge so your history on PRs doesn’t matter. In fact it’s kind of the opposite - I always encourage folks not to rebase and force push once a PR is already in review because you force the reviewer to start from scratch |
|
I’ll give this a review tomorrow, thanks for working through the feedback! |
|
Ok, @JamesHenry noted for next time. I have 1 more rule I would like to propose and on that one I will keep my messy git history once review starts instead of rebasing /force pushing like I did here (eventually). Next rule is also for NgModule and is "no-empty-ngmodule-arrays" Shouldn't take me long now that I have written an eslint rule and understand the process better. |
|
@JamesHenry / @rafaelss95 let me know if you need any more changes (and I won't erase your review history this time) And also thank you for making it possible to so effortlessly write angular linter rules! |
James can have another opinion here, but I'd recommend creating an issue before starting the implementation.
@JamesHenry Looking at it again, would it be weird to call it as Apart of this, from my side everything seems okay. Thank you! :) |
|
@rafaelss95, noted, will create an issue for the next proposed feature |
|
@rafaelss95 @blingerson - sure, let's go for |
Co-authored-by: James Henry <james@henry.sc>
rafaelss95
changed the title
rafaelss95
changed the title
|
Thank you as well! |
This is a best practice we have implemented that simply enforces the sorting of identifiers in ngModule arrays. Not sure if non-codelyzer rules would be acceptable for this project, but thought I would give it a try.