Navigation Menu

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

Add auto-fixing to padded-blocks #6320

Closed
glebmachine opened this issue Jun 4, 2016 · 17 comments
Closed

Add auto-fixing to padded-blocks #6320

glebmachine opened this issue Jun 4, 2016 · 17 comments
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@glebmachine
Copy link

Please, add auto-fixing for padded-blocks option

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jun 4, 2016
@kaicataldo kaicataldo added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 4, 2016
@kaicataldo
Copy link
Member

This seems like a reasonable request to me, though I'm sure where we stand regarding auto-fixing newlines (see #5958 (comment)). Thoughts @eslint/eslint-team?

I'd be happy to take this on if we decide it's a good idea.

@alberto
Copy link
Member

alberto commented Jun 5, 2016

Actually I have a branch where I was trying to implement this, and it lead me to that comment and other concerns raised in #6298. Even if we decide those are not a problem, it's not trivial to remove linebreaks, because they might not be \n.

@glebmachine
Copy link
Author

glebmachine commented Jun 5, 2016 via email

@kaicataldo
Copy link
Member

Whoops, my first response should have read "not sure where we stand..."

@glebmachine
Copy link
Author

Btw, it was great, to have fix for this)

@kaicataldo
Copy link
Member

kaicataldo commented Jun 5, 2016

@alberto Yeah, that makes sense. Any reasons you can think of why the regex solution might not work for removing newlines? Still doesn't solve the problem of adding the right type newline, but that gets back to the discussion happening in the other issue I pointed to earlier (and the one you mentioned).

@glebmachine
Copy link
Author

glebmachine commented Jun 5, 2016

Guys, here is the snippet to detect proper line endings in node:

typeof process != 'undefined' && 'win32' === process.platform ? '\r\n' : '\n'

@nzakas
Copy link
Member

nzakas commented Jun 5, 2016

@glebmachine that isn't accurate. Windows users can use Linux line endings and Linux users can use Windows line endings. You can't make assumptions about this based on the platform.

Removing line breaks can be done with a regex, it's the insertion that is the problem here.

@glebmachine
Copy link
Author

@nzakas In this uncommon case you can check file for used line endings.

@alberto
Copy link
Member

alberto commented Jun 6, 2016

@kaicataldo @nzakas re: removing linebreaks. Unless I am missing something here, it can't be done directly with a regex. To create a fixer you have to provide a range to remove, and you can't get the exact range of the linebreak without resorting to something like what's done in #6226.

@mikesherov
Copy link
Contributor

Could always look how JSCS 2.0 (pre-CST) does this fix.

@lukeapage
Copy link
Contributor

Re: inserting newline, just take the first matching newline in the file or a sensible os default.
Then allow the newline to be fixed by the line break rule in the next iteration for the case where you get it wrong.
Thats my memory of how jscs v2 works (could be wrong)

@mikesherov
Copy link
Contributor

@lukeapage yes, basically.

@kaicataldo
Copy link
Member

@lukeapage @mikesherov Makes sense to me!

@alberto
Copy link
Member

alberto commented Jun 8, 2016

If we decide inserting the wrong linebreak is not a problem to us (even if linebreak-style is disabled!) then yeah, no problem inserting \n.

@kaicataldo
Copy link
Member

As I understand it, it would match the type of the first line break it finds. There is of course the edge case where there might not be any newlines, but that I think that would be pretty rare...

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 14, 2016
@nzakas nzakas closed this as completed in 1c123e2 Jun 15, 2016
@glebmachine
Copy link
Author

Cheers!!

@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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

7 participants