-
-
Notifications
You must be signed in to change notification settings - Fork 243
feat(eslint-plugin): introduce sort keys in type-decorator rule #2187
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
base: main
Are you sure you want to change the base?
feat(eslint-plugin): introduce sort keys in type-decorator rule #2187
Conversation
bc2603e
to
a083356
Compare
packages/eslint-plugin/src/rules/sort-keys-in-type-decorator.ts
Outdated
Show resolved
Hide resolved
It looks like the docs haven't been updated. There are three commands that you can run to update things when a new rule is added (I often forget to do this myself 😆 ):
|
I think this is a good idea. If people are using the schematics to create components, then most of the properties will be in the correct order already.
Looking at the The only think I would suggest is including every property in the defaults, rather than only those that are used by the schematic. For example, there is
schema: [
{
type: 'object',
properties: Object.fromEntries(
Object.keys(AngularClassDecorators).map((key) => [
key,
{
type: 'array',
items: {
type: 'string',
},
},
]),
),
additionalProperties: false,
},
], That produces an options object that looks like this: interface Options {
Component?: string[];
Directive?: string[];
Injectable?: string[];
NgModule?: string[];
Pipe?: string[];
} So you'd have to exclude Component: {
type: 'array',
items: {
type: 'string',
enum: [
'changeDetection',
'viewProviders',
'moduleId',
'templateUrl',
'template',
'styleUrl',
'styleUrls',
'styles',
'etc...',
],
},
}, The only downside to this is there a higher maintenance burden - any time a new property is added to the decorators, the rule needs to be updated. However, the upside is that by defining the schema like this, and also making the |
Thanks! i only saw the update-rule-docs so far :).
Good catch, makes also sense in my opinion. I took a look at the metadata to see for substitutions - the only one was component (having style, styleUrls, styleUIrl and templateUrl, template). I've added them as well to the default options. Thank you also for the inputs regarding the config-schema!
I totaly see that - so I would wait for @JamesHenry s opinion to that matter :). |
hey @bschaeublin, thanks for the PR! |
f393b2d
to
70e6da3
Compare
887bd53
to
47dcdec
Compare
47dcdec
to
b1724d9
Compare
b1724d9
to
0a58476
Compare
@JamesHenry I'm sorry to tag you, as I see that you always got a lot to do, but after hitting the three month mark, I thought I try my luck to get your feedback on this PR (regarding configuration, and general) after I've made some improvements with @reduckted, so that we could complete it :). Thank you! |
I'm really sorry for the delay @bschaeublin. I have to admit it's because I instinctively strongly dislike sorting rules because of how difficult they are to maintain... You can see some of the history here: #1781 Auto-fixing for sorting rules is where the problems really materialize and I see you have added it here. The hugely problematic, but important, thing missing from your tests right now is examples of code comments and how they will be handled during auto-fixing. I got an LLM to assist me with illustrating my point about just how difficult this is on a made up Component: @Component({
/* Multi-line comment above changeDetection (only on one line) */
changeDetection: ChangeDetectionStrategy.OnPush, // Inline comment for changeDetection
// Single-line comment above viewProviders
viewProviders: [
// Inline comment for first provider in viewProviders
{ provide: 'SomeToken', useValue: 'someValue' } /* Multi-line style comment inline after the object */
],
// Comment above moduleId to illustrate its position
moduleId: module.id, // Inline comment specifying moduleId
// Single-line comment above templateUrl
templateUrl: './foo.component.html', /* Inline multi-line comment after templateUrl */
/* Multi-line comment above template.
This template contains an embedded HTML comment. */
template: `<div>
<!-- HTML comment inside template -->
Hello World!
</div>`, // Inline comment after template
// Comment above styleUrl to mark its location
styleUrl: './foo.component.css', // Inline comment for styleUrl
// Comments for styleUrls demonstrating inline and multi-line styles
styleUrls: [
'./foo.component.less', // Inline comment after first styleUrl
'./foo.component.scss' /* Inline comment after second styleUrl */
],
// Comment above styles with an inline style example
styles: [
`/* Inline CSS comment within styles */ body { color: red; }` // Inline comment after style string
],
// Comment above animations to show detailed inline annotations
animations: [
trigger('fade', [
transition(':enter', [
style({ opacity: 0 }),
animate(500, style({ opacity: 1 }))
]) // Inline comment closing the transition array
]) // Inline comment closing the trigger
],
// Comment above encapsulation
encapsulation: ViewEncapsulation.Emulated, // Inline comment for encapsulation setting
/* Multi-line comment above interpolation specifying the markers used */
interpolation: ['{{', '}}'], // Inline comment for interpolation markers
// Single-line comment above preserveWhitespaces
preserveWhitespaces: false, // Inline comment for preserveWhitespaces
// Single-line comment above standalone flag
standalone: true, // Inline comment for standalone component
// Multi-line comment above imports, demonstrating array comments
imports: [
CommonModule, // Inline comment for CommonModule import
FormsModule /* Inline comment for FormsModule import */
],
// Comment above schemas with inline array entry comment
schemas: [
CUSTOM_ELEMENTS_SCHEMA // Inline comment for custom elements schema
],
/* Multi-line comment above override selector to indicate its importance */
'override selector': 'foo-bar', // Inline comment for override selector
// Single-line comment above override inputs
'override inputs': [
'input1', // Inline comment for first input
'input2' /* Inline comment for second input */
],
// Single-line comment above override outputs
'override outputs': [
'output1' // Inline comment for override output
],
// Multi-line comment above override providers with inline object comments
'override providers': [
{
// Inline comment inside the provider object for ServiceA
provide: 'ServiceA',
useClass: class ServiceAImpl {} // Inline comment after useClass declaration
}
],
// Single-line comment above override exportAs
'override exportAs': 'foo', // Inline comment for exportAs
// Multi-line comment above override queries to demonstrate inline property comments
'override queries': {
query1: { read: ElementRef } // Inline comment for query1
},
// Single-line comment above override host
'override host': {
'[attr.role]': 'button' // Inline comment for host attribute
},
// Single-line comment above override jit
'override jit': true, // Inline comment for jit flag
// Multi-line comment above override hostDirectives for clarity
'override hostDirectives': [
{
// Inline comment within host directive object
directive: class DirectiveA {},
inputs: ['inputDirective'], // Inline comment for host directive input
outputs: ['outputDirective'] // Inline comment for host directive output
}
]
})
export class FooComponent {
// Component implementation goes here
} It's really no fun at all to maintain sorting ESLint rules 🥲 I'll keep an open mind, though, and wait to hear back from you on how you get on auto-fixing the sorting of the above with the comments being moved to the appropriate new locations |
0a58476
to
7925aa3
Compare
Thanks @JamesHenry for your response, I saw (to late) your comment in another PR as well, that you didn't like these kind of rules for these (proabably truly) difficult handling of comments. So if you're still open to this like you mentioned, I'd like to search for a stable and nice solution. I think the repository eslint-plugin-perfectionist (which kind soley sorting stuff), do consider comments when re-sorting properties. I saw for example that they work with ranges which contains relevant comments (top, inlinebefore and after), ex. see getNodeRange. I still have to validate that, but if they solved that problem we could do it similar? |
I don't have strong opinions about how you solve it, but the challenge is there on the above snippet and whether it can be proven to be auto-fixable, the above should be your unit test input where you prove that the comments get moved appropriately. Note, formatting doesn't have to be absolutely perfect before and after, but the comments have to belong to the same exact code as before |
@Component({ | ||
/* Multi-line comment | ||
above selector */ | ||
selector: 'app-root', /* Inline multi-line comment after selector */ | ||
// Comment above imports | ||
imports: [ | ||
// Comment inside imports array | ||
CommonModule, // Comment after CommonModule | ||
FormsModule /* Comment after FormsModule */ | ||
], | ||
/* Comment above standalone */ | ||
standalone: true, // Comment after standalone | ||
// Comment above templateUrl | ||
templateUrl: './app.component.html', | ||
/* Multi-line comment | ||
above styleUrl */ | ||
styleUrl: './app.component.css', | ||
// Comment above encapsulation | ||
encapsulation: ViewEncapsulation.None, /* Inline comment for encapsulation */ | ||
// Comment above changeDetection | ||
changeDetection: ChangeDetectionStrategy.OnPush // Inline comment for changeDetection | ||
}) | ||
class Test {} |
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.
This is a more complex combined test based on scenarios from your input:
- It ensures that comments above as well as inline comments get sorted correctly with their associated property (see
changeDetection
) - It also ensures that a comma gets inserted between the property and the comment when it isn't the last property anymore to prevent syntax errors (see
encapsulation
)
All other tests above this ensure dedicated support for multiline comments, inline multi-line comments, and general autofix capabilities.
@JamesHenry Let me know what you think or if you have any other suggestions.
All added tests with maintaining the comments-structure are green currently.
By resorting the properties, newlines between properties are removed by the fixer if there are any between the properties. Let me know if we should support preserving those newlines as well - but that could be seen as a formatting concern rather than a sorting issue.
- Add tests and initial logic for comments - Extract comment utility functions - Add comprehensive test cases with various comment types - Update documentation
1fab9a5
to
a188a6c
Compare
Fixes #797
This PR introduces the ability to sort keys within type decorators (e.g.,
@Component
,@NgModule
, etc.) in thetype-decorator-order
rule.Motivation:
This addresses issue #797, which has been open for a while. I was interested in this functionality and, since it hadn't been implemented yet, I decided to take it on as a way to learn more about ESLint rules, AST parsing, and contribute to the angular-eslint project. This is my first PR in this repository and my first time creating a new ESLint rule, so any guidance and feedback are greatly appreciated!
Changes:
sort-keys-in-type-decorator
to ensure consistent key ordering in type decorators such asComponent
,Directive
,NgModule
, andPipe
.Areas for Discussion/Improvement:
I'd love to get feedback on the following points to ensure this PR aligns well with the project's goals and coding style:
Default Sorts from Angular CLI Schematics:
Using Enums vs. Hardcoded Decorator Sets:
AngularClassDecorators
.I'm eager to learn and improve this PR based on your feedback. Thanks in advance for your time and help!