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

allow overriding the EOL string #89

Merged
merged 1 commit into from
Sep 10, 2017
Merged

Conversation

kellyselden
Copy link
Contributor

Closes #87.

I decided to make it an optional setting as to not cause any grief to anyone. We can always change the default to from '\n' to require('os').EOL in the future if we want to do a major version bump if we consider it a breaking change.

@jprichardson
Copy link
Owner

I just thought of something... does the JSON standard specify an EOL? Or does it state it should be platform specific?

@kellyselden
Copy link
Contributor Author

I'm not sure, but I think once you start involving the file-system, it is your court to format it as you please? Maybe I'm wrong.

I doubt there's a bug in JSON.stringify, so they are probably correctly forcing '\n'. I don't necessarily like that, but it is probably correct to spec.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 6, 2017

The json spec doesn't define any newlines; matter of fact, it doesn't define any formatting; JSON formatting is simply a convention. Here's all the spec has to say:

   Insignificant whitespace is allowed before or after any of the six
   structural characters.

      ws = *(
                %x20 /              ; Space
                %x09 /              ; Horizontal tab
                %x0A /              ; Line feed or New line
                %x0D                ; Carriage return
            )

The EcmaScript spec defines that it shall be a linefeed. I think we should keep this the default; users can override if they wish.

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 6, 2017

One thing I wonder about; is it wise to have this as a global setting on jsonfile.EOL? I know we do this for spaces, but I consider it a bad practice since settings are global to all modules in the Node process.

@kellyselden
Copy link
Contributor Author

I could go either way.

@kellyselden
Copy link
Contributor Author

Is this OK to merge, or should I change the global option?

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 9, 2017

Let's remove the global.

@kellyselden
Copy link
Contributor Author

Global removed

@jprichardson jprichardson merged commit d95fe8b into jprichardson:master Sep 10, 2017
@jprichardson
Copy link
Owner

Thanks @kellyselden!

@kellyselden kellyselden mentioned this pull request Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants