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 issue #707 by adding Atom-based EOL detection #941

Merged
merged 1 commit into from
Apr 23, 2016
Merged

Fix issue #707 by adding Atom-based EOL detection #941

merged 1 commit into from
Apr 23, 2016

Conversation

garretwilson
Copy link
Contributor

What does this implement/fix? Explain your changes.

Fixes issue #707 by adding Atom-based EOL detection.

Does this close any currently open issues?

Closes issue #707 (which was already closed through miscommunication between projects).

Any other comments?

Determines the default line ending based upon the Atom configuration
line-ending-selector.defaultLineEnding. If the Atom configuration
indicates "OS Default", the process.platform is queried, returning
CRLF for Windows systems and LF for all other systems.

Determines the default line ending based upon the Atom configuration
`line-ending-selector.defaultLineEnding`. If the Atom configuration
indicates "OS Default", the `process.platform` is queried, returning
CRLF for Windows systems and LF for all other systems.
@jerone
Copy link

jerone commented Apr 23, 2016

This appears to be not backwards-compatible with build-in line-ending-selector status-bar package.

Can I suggest an fourth option: Auto (or something similar). Which detects the current line-ending per file. Here are some hints to get this in.

@garretwilson
Copy link
Contributor Author

This appears to be not backwards-compatible with build-in line-ending-selector status-bar package.

Please explain specifically what you mean by "not backwards-compatible".

  1. Describe your current configuration/scenario.
  2. Explain the behavior that happens with this pull request.
  3. Indicate the behavior you think should happen instead.

Thanks.

@Glavin001
Copy link
Owner

The failing test is unrelated. Looks good to merge. Thank you for contributing! 😃

@Glavin001 Glavin001 merged commit b86607a into Glavin001:master Apr 23, 2016
@Glavin001
Copy link
Owner

Any improvements to this feature can be submitted in a new Pull Request. Thanks!

@Glavin001 Glavin001 added this to the v0.30.0 milestone Apr 23, 2016
@Glavin001 Glavin001 self-assigned this Apr 23, 2016
@jerone
Copy link

jerone commented Apr 23, 2016

Please explain specifically what you mean by "not backwards-compatible".

The official line-ending-selector allows you to change the line-endings per file. This pull request just checks the current OS default line-ending, which in cross-platform projects may not be true if you have your git checkout line-endings defined as-is.

My suggestion was to actual check the current file for the used line-endings before "forcing" a line-ending.

@garretwilson
Copy link
Contributor Author

Which "line ending setting" is used is a larger issue open to discussion and no doubt differences of opinion. It would be difficult to say that one is right or the other wrong, but rest assured that I did consider whether the current file line ending should be taken into account. I believe the approach I used is correct, and I outline my reasoning below.

As you note the line-ending-selector package indicates the actual line-ending of the current file. The whole purpose of a "beautifier" or "formatter", however, is based on the assumption that the "current" format may not be correct; the beautifier therefore will "normalize" the current file into some "canonical" format.

The question then becomes: what is the "canonical" format that should be used? What better way is it to determine the canonical format than to see which setting the user has used for the Atom configuration?

You mention as a drawback the situation in which a user has configured Git (e.g. using the core.autocrlf property or via a .gitattributes file) to use some fixed line ending and to ignore the system default EOL delimiter (to standardize on LF across platforms, for example). But surely in such a situation, the user would have configured the line-ending-selector package so that instead of "OS Default" it is "LF" or "CRLF".

Put another way, js-beautify should not be checking the setting of the current file or the current setting of the line-ending-selector, but instead its configured default value---and in fact my pull request does exactly that.

@Glavin001
Copy link
Owner

Published to v0.29.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants