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

Implement auto fix functionality for semi-spacing rule #3829

Closed
alberto opened this Issue Sep 16, 2015 · 17 comments

Comments

Projects
None yet
4 participants
@alberto
Copy link
Member

alberto commented Sep 16, 2015

Implement auto fix functionality for semi-spacing rule

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Sep 16, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Sep 16, 2015

@gyandeeps gyandeeps added enhancement rule and removed triage labels Sep 16, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 17, 2015

I'd like to hold off on this one for a little bit before implementing. I'm a bit concerned about overlap with the semi rule fixes. So I'll mark as accepted, but would like to wait a release or two before merging.

@nzakas nzakas added the accepted label Sep 17, 2015

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Sep 17, 2015

@nzakas are you ok with me implementing any other spacing fixes or do you want to put all of them on hold for now?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 18, 2015

Just this one. However, if you want to implement auto-fixing in a rule, file an issue and wait for it to be accepted before starting work. We are intentionally going slow on this to account for bugs we will find a long the way, so we don't want to add a lot of auto-fixing rules at once.

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Sep 18, 2015

Understood
El 18/09/2015 17:46, "Nicholas C. Zakas" notifications@github.com
escribió:

Just this one. However, if you want to implement auto-fixing in a rule,
file an issue and wait for it to be accepted before starting work. We are
intentionally going slow on this to account for bugs we will find a long
the way, so we don't want to add a lot of auto-fixing rules at once.


Reply to this email directly or view it on GitHub
#3829 (comment).

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Dec 15, 2015

@nzakas what's your take on this? I don't think the fix introduces any overlap.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 15, 2015

Can you explain why you think there's no overlap?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 15, 2015

Or rather, no conflicts?

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Dec 15, 2015

I think there is already a possible conflict, where a semi is needed and autofixed by the semi rule, but not according to the config of semi-spacing. Adding autofixing for semi-spacing doesn't fix this, but I don't see how it would create any other conflict. Maybe you see something that I am missing here.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 16, 2015

We shouldn't add more conflicts just because some already exist. What I'm looking for here is examples showing this would not conflict or that it would.

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Dec 17, 2015

The thing is I don't think adding the fix causes a conflict, but giving a couple of examples showing no conflicts doesn't prove that. When do you think they would cause a conflict?

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Dec 17, 2015

The only case I can think of is if you set semi to never and you also have semi-spacing enabled. It could cause conflicts if you have unneeded semicolons. But it doesn't make sense to me to enable semi-spacing in that case, since it wouldn't do anything.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

Theoretically, any inserted semicolon could also require a space before it. Those warnings would be missed when the semicolon is inserted. I suppose we could limit the fix to be only when there is no space required before the semicolon?

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Dec 18, 2015

Yes it could, but that is already happening (it's the one I was refering to here). That's caused by semi's autofix, not by the addition of the fix to semi-spacing.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

You've lost me, what do you mean "that is already happening"? What is already happening?

@alberto

This comment has been minimized.

Copy link
Member Author

alberto commented Dec 18, 2015

What you just said. :) semi is already inserting missing semicolons. If semi-spacing is configured to require a space before the semicolon, that warning will be missed.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 18, 2015

Ah, good point. Okay, go for it.

@alberto alberto closed this in f8a1e47 Dec 19, 2015

nzakas added a commit that referenced this issue Dec 19, 2015

Merge pull request #3830 from alberto/issue3829
Update: Implement auto fix for semi-spacing rule (fixes #3829)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

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