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

Bug: eol-last autofix doesn't take linebreak-style into account #4148

Closed
alberto opened this Issue Oct 15, 2015 · 16 comments

Comments

Projects
None yet
7 participants
@alberto
Member

alberto commented Oct 15, 2015

eol-last autofix always inserts '\n' at the end of the file regardless of the file's linebreak-style

e.g.
input

var a = 123;\r\nvar b = 456;

expected

var a = 123;\r\nvar b = 456\r\n;

result

var a = 123;\r\nvar b = 456\n;
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Oct 15, 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 commented Oct 15, 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 Oct 15, 2015

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Oct 15, 2015

Member

Yes, we still haven't really figured out how to do cross-rule fixes. Not really sure how we can accomplish this without creating a lot of cross-depandant code.

Member

ilyavolodin commented Oct 15, 2015

Yes, we still haven't really figured out how to do cross-rule fixes. Not really sure how we can accomplish this without creating a lot of cross-depandant code.

@platinumazure

This comment has been minimized.

Show comment
Hide comment
@platinumazure

platinumazure Oct 15, 2015

Member

At least it should be possible (if annoying) to add the linebreak delimiter to both linebreak-style and eol-last, right?

Member

platinumazure commented Oct 15, 2015

At least it should be possible (if annoying) to add the linebreak delimiter to both linebreak-style and eol-last, right?

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 15, 2015

Member

As it is right now, this autofix is incomplete, and can cause new errors that would be hidden. @nzakas was oppossed to that in another thread (I can't find it now). So I guess the autofix should be removed.

Member

alberto commented Oct 15, 2015

As it is right now, this autofix is incomplete, and can cause new errors that would be hidden. @nzakas was oppossed to that in another thread (I can't find it now). So I guess the autofix should be removed.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Oct 15, 2015

Member

Agree. I think until we can figure out a way to share this type of information cross-rule, we should remove autofix from this rule.

Member

ilyavolodin commented Oct 15, 2015

Agree. I think until we can figure out a way to share this type of information cross-rule, we should remove autofix from this rule.

@ilyavolodin ilyavolodin added bug rule accepted and removed triage labels Oct 15, 2015

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Oct 15, 2015

Member

I don't know why but I would give this rule leeway to remain fixable.
I know we don't do multiple runs but if line-ending rule is configured correctly then it can fix this issue (if needed) in the second run. Just a thought, don't have any strong opinion here.

Member

gyandeeps commented Oct 15, 2015

I don't know why but I would give this rule leeway to remain fixable.
I know we don't do multiple runs but if line-ending rule is configured correctly then it can fix this issue (if needed) in the second run. Just a thought, don't have any strong opinion here.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 15, 2015

Member

Let's just let people pass in the linebreak style to this rule in the short-term. No need to remove autofixing.

Member

nzakas commented Oct 15, 2015

Let's just let people pass in the linebreak style to this rule in the short-term. No need to remove autofixing.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 15, 2015

Member

It should then be a required option, right? Otherwise it can't be autofixed correctly, we can't assume a default. Doesn't that make this a breaking change?

Member

alberto commented Oct 15, 2015

It should then be a required option, right? Otherwise it can't be autofixed correctly, we can't assume a default. Doesn't that make this a breaking change?

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Oct 15, 2015

Member

@nzakas this setting could be still different from the setting of linebreak-style.

Member

lo1tuma commented Oct 15, 2015

@nzakas this setting could be still different from the setting of linebreak-style.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 15, 2015

Member

@alberto we default linebreak-style to unix. We could do the same here.

@lo1tuma yeah, that's up to the user to fix.

Keep in mind that removing autofixing is a breaking change. :)

Member

nzakas commented Oct 15, 2015

@alberto we default linebreak-style to unix. We could do the same here.

@lo1tuma yeah, that's up to the user to fix.

Keep in mind that removing autofixing is a breaking change. :)

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Oct 15, 2015

Member

yeah, that's up to the user to fix.

The problem is that the user wouldn’t notice this problem without a second run.

Member

lo1tuma commented Oct 15, 2015

yeah, that's up to the user to fix.

The problem is that the user wouldn’t notice this problem without a second run.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 15, 2015

Member

@nzakas I meant breaking (assuming the option was required) as in the original rule config would then be invalid. If we can assume a default I'm ok with that

Member

alberto commented Oct 15, 2015

@nzakas I meant breaking (assuming the option was required) as in the original rule config would then be invalid. If we can assume a default I'm ok with that

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 16, 2015

Member

I updated the PR with the added linebreak style config option.

Member

alberto commented Oct 16, 2015

I updated the PR with the added linebreak style config option.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 16, 2015

Member

@lo1tuma it's the same problem we have right now, only with the possibility of it being better

Member

nzakas commented Oct 16, 2015

@lo1tuma it's the same problem we have right now, only with the possibility of it being better

@lo1tuma

This comment has been minimized.

Show comment
Hide comment
@lo1tuma

lo1tuma Oct 16, 2015

Member

Yes, it would be better as the current situation but it won't fix the problem.

Member

lo1tuma commented Oct 16, 2015

Yes, it would be better as the current situation but it won't fix the problem.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 16, 2015

Member

It won't automatically fix the problem, yes, but it will give people the opportunity to fix it themselves until we can figure out how to share such settings across rules.

Member

nzakas commented Oct 16, 2015

It won't automatically fix the problem, yes, but it will give people the opportunity to fix it themselves until we can figure out how to share such settings across rules.

@alberto alberto closed this in 3d1086c Oct 23, 2015

nzakas added a commit that referenced this issue Oct 23, 2015

Merge pull request #4151 from alberto/issue4148
Update: Add linebreak style option to eol-last (fixes #4148)

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