New: Add --fix-type option to CLI (fixes #10855)

[nzakas]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[x] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Implemented the --fix-type option as described in #10855. Also included a tool for easily updating the type of each rule based on the rule-types.json file.

As for approach, I added a fixTypes property to the CLIEngine configuration. I chose to do this rather than overwriting the fix option. I think fixTypes is a better implementation because it exposes this functionality to code editors whereas just overriding the fix option would not have done so.

Is there anything you'd like reviewers to focus on?

1. Did I properly implement the change?
2. Did I write all appropriate tests?
3. Review rule-types.json to see if you disagree with any of the assigned types.

Note: The first commit on this PR contains the default rule-types.json that I created based solely on matching meta.docs.category. The second commit (which I will add soon) is my suggested changes.

[not-an-aardvark]

Thanks for working on this! I left a few comments on the implementation and quickly skimmed through the rule list, although I haven't gone through the rules thoroughly yet.

One thing I noticed is that aside from no-extra-boolean-cast, none of the rules marked as "problem" are autofixable. (I think no-extra-boolean-cast should probably be considered a suggestion.) This makes me wonder about the utility of --fix-type=problem given that most errors in code won't be fixable, although I'm fine with keeping it for consistency with the other types.

[not-an-aardvark]

Bikeshed: I think we currently use the term "problem" to mean "a warning or error reported by ESLint". For example, the default formatter outputs a message along the lines of ✖ 1 problem (1 error, 0 warnings).

I'm wondering if using the term "problem" to refer to potential errors could lead to confusion, because when a stylistic rule creates a report, it's also called a "problem" with the current terminology. Is there a term we could use other than "problem" to describe reports that address potential errors in code? (We've also used "messages" in some places to refer to reported errors/warnings, although this is also confusing because it sometimes refers to the text associated with a problem rather than the problem itself.)

[nzakas]

Yeah, I also went back and forth on this, and believe that "problem" is the correct term. Some of the other terms we use:

• "error" is a severity level, so definitely can't use that
• We use "messages" in the output data structure as a generic catchall for everything, so we don't need to worry about that.
• As best I can tell, "problem" is only ever used in the default formatter output. My feeling is that this isn't too confusing, but if others feel it is, I'd suggest changing the term to "messages" to echo what is in the data structure.

[platinumazure]

Should we change the stylish formatter? We could use "issue" there, for example, whereas "issue" doesn't work as well for the rule type (IMO).

(I'm aware "issue" also has meaning in GitHub, but I'm not worried about confusion between lint output and GitHub at this point.)

[nzakas]

I wouldn't want to change the formatter as part of this PR, as I think that's a breaking change (we never know who is parsing the output). I'm open to changing it, though as I said, I also think it's fine if we don't.

[kaicataldo]

problem feels better to me than messages because messages sounds all-encompassing - it's not immediately clear how messages differs from suggestion or style

[kaicataldo]

Thanks for working on this!

[nzakas]

I've updated the rule types based on feedback. Please take a look at the most recent commit and comment on rule-types.json if you have further feedback.

[nzakas]

So, after synthesizing all of the feedback about rule types, it seems clear to me that the word "style" is confusing pretty much everyone. I think that I've tried to redefine "style" to make it more narrow than most people seem to use, and if we are already having trouble with it, it might not be the best choice.

Updated Proposal

I think we can make the rule types less confusing if we change "style" to "layout". That would mean we have these three categories:

1. problem - flags code that could cause an error
2. suggestion - flags code that could be done in a better way
3. layout - flags code related only to whitespace changes in between AST nodes

The "layout" type would be more restrictive than the current "style", but I think doing so makes the intent clearer.

Thoughts?

[platinumazure]

I like the intent of narrowing the focus of style changes and promoting some current "style" rules to suggestions.

That said: Should token presence/absence or location (where there are no changes to AST besides location info) also fall under "layout", or should those be suggestions? (E.g., semi, semi-style, comma-dangle, comma-style) Or have I just reopened the previous can of worms?

[nzakas]

The intent here is that "layout" only refers to whitespace. Semicolons, parentheses, etc., would then fall under "suggestion" (which is where the confusion between style vs. suggestion seems to be right now).

@kaicataldo @ilyavolodin @not-an-aardvark do you have thoughts on this?

[not-an-aardvark]

I don't feel extremely strongly about it, but I still think a "non-AST fixes only" mode would be useful for the reasons described in #10912 (comment).

Re. the distinction between changing the structure of the AST versus the value of the AST nodes, I'm not sure it's useful to draw a line between the two. For example, if I changed the name of a Identifier variable reference to refer to a different variable which is also defined in scope, then I would be significantly changing the semantics of a program even though I would only have changed the value of an AST node rather than the structure of the AST.

[nzakas]

@not-an-aardvark yeah, this is what I'm trying to get towards: a setting that doesn't touch the AST at all and also isn't confused by the terminology. I think "layout" isn't the best word, but we're not using it anywhere else so I think it's unambiguous and we can define it exactly how we want.

[not-an-aardvark]

I'm okay with using the term "layout" -- I was more trying to suggest that we should consider things like semicolons and parentheses as "layout" issues rather than suggestions since they don't change the AST.

[nzakas]

We could certainly consider semicolons and parentheses as part of layout, I'm not opposed to that and I think wouldn't be too confusing.

…

On Wed, Oct 24, 2018 at 10:14 AM Teddy Katz ***@***.***> wrote: I'm okay with using the term "layout" -- I was more trying to suggest that we should consider things like semicolons and parentheses as "layout" issues rather than suggestions since they don't change the AST. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10912 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWktgxcYHT7HyLXlEA38kP-YkaPlCjks5uoJ_5gaJpZM4XE4bQ> .

--

______________________________ Nicholas C. Zakas @SlickNet Author, Principles of Object-Oriented JavaScript <http://amzn.to/29Pmfrm> Author, Understanding ECMAScript 6 <http://amzn.to/29K1mIy>

[kaicataldo]

I agree with @not-an-aardvark about having a category that is "non-AST fixes only". It would allow ESLint to be used in conjunction with other tooling in the current greater JS ecosystem (specifically Prettier).

I think "layout" works, but I do wonder if that holds up if we start including semicolons and parentheses as part of the category. The only other term I can think of is "style", but then we're back to where we started!

I would be fine with either "layout" or "style".

[nzakas]

I think the nice thing about "layout" is that it doesn't have a tightly-coupled meaning to code at this point, so we are free to define it however we want. "style" means a lot of things to a lot of different people, so that becomes a more difficult term to redefine in an ESLint-specific way.

I'm going to go with "layout" as meaning whitespace, semicolons, commas, and parentheses and update this PR. Then we can do a final review and see if everyone thinks it makes sense.

[nzakas]

Okay, I've reviewed each rule and made adjustments based on the last proposal. I really like this as I think it greatly clarified which type of rule each rule should be.

Please leave comments on rule-types.json if you think there is a mistake (this will make it easier for us all to load this ever-growing PR to review).

[nzakas]

@eslint/eslint-tsc sorry to ping everyone, but I'm having trouble keeping this PR updated because it touches so many files. I believe this is ready for a final review, so I'd appreciate people taking a look and letting me know what you think.