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

Updated prettydiff.coffee to support EOL Detection #1461

Closed
wants to merge 1 commit into from
Closed

Updated prettydiff.coffee to support EOL Detection #1461

wants to merge 1 commit into from

Conversation

peeratant
Copy link
Contributor

What does this implement/fix? Explain your changes.

As stated in Issue 1457, Pretty Diff required "crlf" parameter to set EOL type.

Does this close any currently open issues?

Issue 1457

Previously,this error had been reported by affanshahid in Issue 1009

Any other comments?

This error used the same logic in Issue 707 with some minor changes.

@peeratant peeratant changed the title Updated PrettyDiff.coffee to support EOL Detection Updated prettydiff.coffee to support EOL Detection Jan 11, 2017
@@ -117,6 +117,7 @@ module.exports = class PrettyDiff extends Beautifier
source: text
lang: lang
mode: "beautify"
crlf: getDefaultLineEnding() # solved issue 1457
Copy link
Owner

Choose a reason for hiding this comment

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

The commit should reference the issue, not the code.

  • Remove comment

Copy link
Owner

Choose a reason for hiding this comment

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

Although I see now another commit also incorrect did this:

options.eol = getDefaultLineEnding() ? options.eol #fixes issue #707

# configuration, or `null` if the Atom line ending configuration was not
# recognized.
# see: https://github.com/atom/line-ending-selector/blob/master/lib/main.js
getDefaultLineEnding= ->
Copy link
Owner

Choose a reason for hiding this comment

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

  • Can you move this into beautifier.coffee along with
    # Retrieves 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.
    # Code modified from atom/line-ending-selector
    # returns: The correct line-ending character sequence based upon the Atom
    # configuration, or `null` if the Atom line ending configuration was not
    # recognized.
    # see: https://github.com/atom/line-ending-selector/blob/master/lib/main.js
    getDefaultLineEnding= ->
    switch atom.config.get('line-ending-selector.defaultLineEnding')
    when 'LF'
    return '\n'
    when 'CRLF'
    return '\r\n'
    when 'OS Default'
    return if process.platform is 'win32' then '\r\n' else '\n'
    else
    return null
    ?
    Since they are used in multiple places it should be centralized within the beautifier class.

@@ -117,6 +117,7 @@ module.exports = class PrettyDiff extends Beautifier
source: text
lang: lang
mode: "beautify"
crlf: getDefaultLineEnding() # solved issue 1457
Copy link
Owner

Choose a reason for hiding this comment

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

@prettydiff can you confirm the option is named crlf? That seems strange to me, instead of eol.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that is the correct option name and yes eol is a more intelligent name. http://prettydiff.com/documentation.xhtml#crlf

@@ -117,6 +117,7 @@ module.exports = class PrettyDiff extends Beautifier
source: text
lang: lang
mode: "beautify"
crlf: getDefaultLineEnding() # solved issue 1457
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

  • Update options documentation to reflect the eol option support in Pretty Diff

Copy link
Owner

@Glavin001 Glavin001 Jan 11, 2017

Choose a reason for hiding this comment

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

@Glavin001 Glavin001 self-assigned this Jan 11, 2017
@Glavin001 Glavin001 added this to the v0.30.0 milestone Jan 11, 2017
@peeratant
Copy link
Contributor Author

peeratant commented Jan 12, 2017

Okay, I will try to finish this within tomorrow if possible. (Today, I have to clear out my work first)

Check list

  • Remove comment
  • Add eol option for js, jsx
  • Centralize the code

I think I will have to create things like dictionary as well to support alias option name and value.

For example, Create config.json which contains alias data for specific option and make function to convert passing args base on this file.

Ex. Map
Alias name - eol to crlf
Alias val - CRLF ( true ) / LF ( false )

I will submit a pull request first once I finish and then we will discuss about this later.

@Glavin001
Copy link
Owner

I think I will have to create things like dictionary as well to support alias option name and value.

For example, Create config.json which contains alias data for specific option and make function to convert passing args base on this file.

Ex. Map
Alias name - eol to crlf
Alias val - CRLF ( true ) / LF ( false )

I do not follow what you mean. I do not want you to accidentally do unneeded work so I wanted to make sure you know Atom-Beautify supports different types of options:

I expect you to have something like "crlf": "eol" option in Pretty Diff and add eol (boolean) option to the respective languages.

I hope that helps!

@peeratant peeratant closed this Jan 14, 2017
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