Update: add ignoreComments option to indent rule (fixes #9018)
#9752
Conversation
platinumazure
added
accepted
There was a problem hiding this comment.
LGTM
Is there a more idiomatic way I should have done this?
Another option would have been to add an early return here. That would have the semantics of "just don't report this token if there would have been an error for it", whereas ignoreToken does something slightly more complex (described here). I don't think there is a difference in the case of comments (since no other tokens depend on the indentation of comments IIRC), but it seems like the ignoreToken approach is semantically closer to what we want, so I think your solution is better than that alternative.
Any advice on how to test performance and make sure it's acceptable?
You could use TIMING=1 npm run lint before and after to see if indent takes a substantially larger percentage of rule time with this change applied, when linting the ESLint codebase. I don't think this change would pose a problem, but I agree that it's a good idea to check.
Additional performance-measuring commands (probably not as useful for this case, but good to know in general):
npm run perfruns all rules on a large file, and times the process. (Unfortunately this benchmark has a significant amount of noise due to I/O time, and it reports significantly more errors than a typical workload -- see Improve performance benchmark #9184)- When refactoring
Linter, I found thattime npm run lintwas also useful as a way to get a more realistic workload. (Since this lints the ESLint codebase itself, it could sometimes cause anomalies, e.g. if a change adds a large file, the linting time would increase because there is more text to lint, even if the change is performant. This could be avoided by ignoring the changed file.)
| @@ -84,7 +84,8 @@ This rule has an object option: | |||
| * `"ObjectExpression"` (default: 1) enforces indentation level for properties in objects. It can be set to the string `"first"`, indicating that all properties in the object should be aligned with the first property. This can also be set to `"off"` to disable checking for object properties. | |||
| * `"ImportDeclaration"` (default: 1) enforces indentation level for import statements. It can be set to the string `"first"`, indicating that all imported members from a module should be aligned with the first member in the list. This can also be set to `"off"` to disable checking for imported module members. | |||
| * `"flatTernaryExpressions": true` (`false` by default) requires no indentation for ternary expressions which are nested in other ternary expressions. | |||
| * `ignoredNodes` accepts an array of [selectors](/docs/developer-guide/selectors.md). If an AST node is matched by any of the selectors, the indentation of tokens which are direct children of that node will be ignored. This can be used as an escape hatch to relax the rule if you disagree with the indentation that it enforces for a particular syntactic pattern. | |||
| * `"ignoredNodes"` accepts an array of [selectors](/docs/developer-guide/selectors.md). If an AST node is matched by any of the selectors, the indentation of tokens which are direct children of that node will be ignored. This can be used as an escape hatch to relax the rule if you disagree with the indentation that it enforces for a particular syntactic pattern. | |||
| * `"ignoreComments"` (default: false) can be used when comments do not need to be aligned with nodes on the previous or next line. | |||
There was a problem hiding this comment.
I'm wondering if we should document the existing comment behavior about allowing alignment with the previous/next line. It seems like this description hints at the idea, but I'm not sure if it's explained more thoroughly anywhere.
There was a problem hiding this comment.
The word "comment" isn't even mentioned in the docs currently, so I would hazard a guess that there is no description of expected comment indentation. If you want me to add that to this pull request, feel free to Request Changes here.
There was a problem hiding this comment.
¯\_(ツ)_/¯ It's not a big problem, I'm fine with this being merged as-is.
|
what about block comments? /* block-line comment */
var a = 1;
/*
block comment
*/
var a = 1; |
|
@gyandeeps I think this change should work for block comments too (and I think the rule currently does not check for alignment of text in later lines of the block comment, but I'll check). I'll add tests for block comments. @not-an-aardvark Thanks, I'll do some performance tests after I make the suggested changes. |
|
Apologies for not leaving an update sooner. I've been doing Christmas shopping last-minute 😆 Block commentsI added basic tests for just checking the alignment of start of block comment. @gyandeeps Given you approved the PR, hopefully that looks good to you. However, I wasn't sure if you asking about indentation/alignment of consecutive lines in a comment. If so, I haven't tested that. At the time, I was thinking That said, I don't think indent currently handles those continuing lines in block comments, so I don't particularly want to add a lot of extra logic in this PR. PerformanceI didn't capture the metrics, but I did follow @not-an-aardvark's suggestions and saw that performance was unchanged. I even tried changing our lint settings to use Let me know if I should run any more tests or show some metrics. |
|
@eslint/eslint-team Anyone else want to review this? |
ghost
mentioned this pull request
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
See #9018.
What changes did you make? (Give an overview)
Added
ignoreCommentsoption (default: false) to indent rule.Is there anything you'd like reviewers to focus on?
This seemed, um, simpler than I thought. Is there a more idiomatic way I should have done this? Any advice on how to test performance and make sure it's acceptable?