jprichardson / node-jsonfile

Easily read/write JSON files.
MIT License
1.2k stars 321 forks source link

allow overriding the EOL string #89

Closed kellyselden closed 7 years ago

kellyselden commented 7 years ago

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 commented 7 years ago

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

kellyselden commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

I could go either way.

kellyselden commented 7 years ago

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

RyanZim commented 7 years ago

Let's remove the global.

kellyselden commented 7 years ago

Global removed

jprichardson commented 7 years ago

Thanks @kellyselden!