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

Fix: func-call-spacing "never" doesn't fix w/ line breaks (fixes #7787) #7788

Merged
merged 1 commit into from Dec 19, 2016

Conversation

platinumazure
Copy link
Member

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

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

See #7787.

What changes did you make? (Give an overview)

With "func-call-spacing": ["error", "never"], ensure that autofixing only occurs if there are no line terminators in the spacing. This is to avoid conflicts with no-unexpected-multiline, which is indispensable for identifying potential problems in an ASI codebase.

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

Two questions:

  1. Should I update documentation to add auto-fix caveats (i.e., list what won't be fixed)? If so, do we have a standard way of doing that? (I'm not sure I need to because we never guarantee that all violations are fixable.)
  2. Technically the warning count is unchanged but the perceived warning count would go up due to some violations no longer being auto-fixed. Is this still semver-patch since the actual pre-autofix warning count is unchanged, or should this be semver-minor? (I know this will be released semver-minor anyway, but I'm asking if I need to change the commit prefix to Update:.)

@mention-bot
Copy link

@platinumazure, thanks for your PR! By analyzing the history of the files in this pull request, we identified @btmills, @kaicataldo and @scriptdaemon to be potential reviewers.

@eslintbot
Copy link

LGTM

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules labels Dec 17, 2016
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Should I update documentation to add auto-fix caveats (i.e., list what won't be fixed)? If so, do we have a standard way of doing that? (I'm not sure I need to because we never guarantee that all violations are fixable.)

I think it's probably fine as-is, but I wouldn't be opposed to updating it. I don't think we have a standard way to do that (and most rules currently don't do it at all), but for eqeqeq I just put a note in the "Rule Details" section.

Technically the warning count is unchanged but the perceived warning count would go up due to some violations no longer being auto-fixed. Is this still semver-patch since the actual pre-autofix warning count is unchanged, or should this be semver-minor? (I know this will be released semver-minor anyway, but I'm asking if I need to change the commit prefix to Update:.)

I think this is semver-patch (and should have the Fix: prefix), because we don't make any guarantees about what gets autofixed by a rule. (This is unlikely to affect most build systems anyway, because people typically only run --fix while developing, and then run without --fix in CI.)

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@kaicataldo kaicataldo merged commit 428fbdf into master Dec 19, 2016
@platinumazure platinumazure deleted the issue7787 branch December 19, 2016 22:23
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants