Skip to content
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

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

Merged
merged 25 commits into from Nov 6, 2018

Conversation

@nzakas
Copy link
Member

commented Oct 2, 2018

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.

@eslint eslint bot added the triage label Oct 2, 2018

@nzakas nzakas added cli feature and removed triage labels Oct 2, 2018

@not-an-aardvark
Copy link
Member

left a comment

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.

lib/cli-engine.js Outdated Show resolved Hide resolved
lib/cli-engine.js Outdated Show resolved Hide resolved
lib/options.js Outdated Show resolved Hide resolved
lib/cli-engine.js Outdated Show resolved Hide resolved

This option allows you to specify the type of fixes to apply when using either `--fix` or `--fix-dry-run`. The three types of fixes are:

1. `problem` - fix potential errors in the code

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 3, 2018

Member

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.)

This comment has been minimized.

Copy link
@nzakas

nzakas Oct 3, 2018

Author Member

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.

This comment has been minimized.

Copy link
@platinumazure

platinumazure Oct 3, 2018

Member

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.)

This comment has been minimized.

Copy link
@nzakas

nzakas Oct 4, 2018

Author Member

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.

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Oct 17, 2018

Member

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

lib/rules/no-undef.js Outdated Show resolved Hide resolved
lib/rules/no-extra-boolean-cast.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
lib/rules/default-case.js Show resolved Hide resolved
lib/rules/no-fallthrough.js Outdated Show resolved Hide resolved

@nzakas nzakas added do not merge and removed do not merge labels Oct 3, 2018

Makefile.js Show resolved Hide resolved
lib/rules/camelcase.js Outdated Show resolved Hide resolved
lib/rules/consistent-this.js Outdated Show resolved Hide resolved
lib/rules/default-case.js Show resolved Hide resolved
lib/rules/func-style.js Outdated Show resolved Hide resolved
lib/rules/key-spacing.js Show resolved Hide resolved
lib/rules/rest-spread-spacing.js Outdated Show resolved Hide resolved
lib/rules/sort-imports.js Show resolved Hide resolved
lib/rules/sort-keys.js Outdated Show resolved Hide resolved
lib/rules/yield-star-spacing.js Outdated Show resolved Hide resolved
tests/lib/cli.js Show resolved Hide resolved

@nzakas nzakas force-pushed the issue10855 branch from 4453327 to 65bd89a Oct 17, 2018

lib/rules/no-empty.js Outdated Show resolved Hide resolved
@kaicataldo
Copy link
Member

left a comment

Thanks for working on this!

@nzakas nzakas added the do not merge label Oct 19, 2018

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

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 nzakas removed the do not merge label Oct 30, 2018

@nzakas nzakas force-pushed the issue10855 branch from 3da86f2 to dcb5d05 Nov 1, 2018

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2018

@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.

@nzakas nzakas force-pushed the issue10855 branch from 4f992a6 to 5e1c448 Nov 6, 2018

@nzakas nzakas merged commit 7ad86de into master Nov 6, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@nzakas nzakas deleted the issue10855 branch Nov 6, 2018

@eslint eslint bot locked and limited conversation to collaborators May 6, 2019

@eslint eslint bot added the archived due to age label May 6, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.