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

feat: append eol to package.json #38

Closed
wants to merge 3 commits into from

Conversation

nexdrew
Copy link
Member

@nexdrew nexdrew commented May 21, 2016

Fixes #37.

I'm thinking we should probably replace all \n characters with os.EOL to be more Windows friendly. I separated the commits so I can easily revert the 2nd one if you disagree.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 95.238% when pulling c63fb34 on nexdrew:eol into 013b145 on conventional-changelog:master.

@bcoe
Copy link
Member

bcoe commented May 21, 2016

👍 no objection from me :shipit:

@bcoe
Copy link
Member

bcoe commented May 21, 2016

@nexdrew made you an owner of the npm package FYI.

@stevemao
Copy link
Member

Would this cause inconsistent eol? Or we don't want to enforce it?

@nexdrew
Copy link
Member Author

nexdrew commented May 22, 2016

@stevemao Not sure I follow you. What do you mean by "inconsistent eol"? Do you mean changing it b/w Windows and *nix runs?

Do you think we should try to detect which eol the files have (like Atom) before rewriting/appending to them? That would probably be better.

@stevemao
Copy link
Member

If i ran the command on windows and later on i run the same command on
linux, there would be a change because of different eol. Not sure if this
is expected?
On 22 May 2016 12:00 PM, "Andrew Goode" [email protected] wrote:

@stevemao https://github.com/stevemao Not sure I follow you. What do
you mean by "inconsistent eol"? Do you mean changing it b/w Windows and
*nix runs?

Do you think we should try to detect which eol the files have (like Atom)
before rewriting/appending to them? That would probably be better.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#38 (comment)

@nexdrew
Copy link
Member Author

nexdrew commented May 22, 2016

@stevemao Yeah, that's a good point. I'll change it to detect existing eol instead of always using the platform-specific one.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 95.506% when pulling 38bcf34 on nexdrew:eol into 013b145 on conventional-changelog:master.

@nexdrew
Copy link
Member Author

nexdrew commented May 22, 2016

I seem to have opened a small can of worms here. It works as expected for package.json (though we might want to consider also using detect-indent to preserve indentation too) but I'm not too sure about CHANGELOG.md since conventional-changelog always uses \n (I think), which seems less important given that the content there should always be generated.

If you think this is too much, I'd be ok simplifying this to just append \n to package.json and splitting the EOL preservation into another PR. Definitely a little scope creep here.

@stevemao
Copy link
Member

Well conventional-changelog uses templates so yeah it should be always \n. I never came across require('os').EOL before and I'm not sure about when to use it, when not to.

@nexdrew
Copy link
Member Author

nexdrew commented May 23, 2016

Closing in favor of #42 (fix without scope creep) and #43 (new issue to discuss problems encountered here).

@nexdrew nexdrew closed this May 23, 2016
@nexdrew nexdrew deleted the eol branch May 23, 2016 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants