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 config a new line at end of extracted file #58

Conversation

michelts
Copy link
Contributor

Hi @gilbsgilbs

We use vim to update the translations files and it adds a newline char at the end of the file automatically.

You are writing the json-stringified content directly, without this new line. Do you mind if we have a config option to include this new line char?

@gilbsgilbs
Copy link
Owner

gilbsgilbs commented Aug 26, 2019

Hi and thanks for your contribution.

The implementation looks very clean to me however I'm a bit bothered by the fact it adds another (very specific) option to the long list of existing configuration options.

What I think is problematic overall is really the fact that the JSONv3 exporter doesn't preserve orders, newlines and whitespaces from the original file. Unfortunately, I haven't found a JSON parser/serializer that have such feature yet. To be honest, I have already been bitten by a similar issue with Weblate because it doesn't use the same sorting algorithm as this library and that causes huge diffs that are impractical to work with and prone to conflicts. That's something I'd definitely like to address at some point.

To be clear: I would prefer a PR that modifies the JSONv3 exporter to memorize trailing whitespaces and newlines from the imported file and restore them later since that goes towards the direction of being less destructive against the exported file. Fortunately, #48 may make this work slightly more simple. (By the way, another intention of #48 was also to get closer to allowing third-party exporters which would also solve your problem, but we're not quite there yet.)

Another option I can suggest to you is to add the trailing newline yourself in your build scripts, although I can hear it's a bit more cumbersome than having this built-into the library.

What do you think?

@michelts
Copy link
Contributor Author

Hi @gilbsgilbs

I would bother that much with a non destructive operation. All other js parsers I've tested are destructive, reorder the keys and trim white spaces. Even the ones that doesn't generate a json file, for instance, the django parser that generates gettext files also would format my file according his own pattern (adding line breaks and removing white spaces).

I agree it would be cool to have a non destructive parsing operation, specially when integrating with 3rd part softwares like Weblate, but it seems to be a different thing from the new line at the end of the file.

I've added an option to include the new line at the end of the file to keep backward compatibility and avoid existing projects to have their translation files changed, but I think having the new line should be the default, since that's the POSIX standard.

There are other softwares that generate json files that faced the same discussion, like netlify or serverless, although I don't know/use any one of them.

I can include the newline char in my build process, but my opinion is that we should include support for the newline in the main plugin, and I would go further, I would make it the default 😃

@gilbsgilbs
Copy link
Owner

gilbsgilbs commented Aug 26, 2019

I opened #59 to keep track of this (with some ideas for a solution). I hope you'll understand that I'm not very inclined to merge this PR when what I consider to be the target solution would probably require to remove this configuration option. (Again, this doesn't mean it is not a good and clean work 🙏.)

I reckon keeping track of trailing whitespaces in the exporter is a good compromise that 1) fixes your issue 2) neither require too much work nor adds too much complexity 3) goes toward the good direction without requiring a later BC depending on what we decide to do or not in the future.

I'm currently on vacation 🌴, but I'll probably fix this as soon as I'm back (if you don't). Sorry for the delay and for being so conservative.

@michelts
Copy link
Contributor Author

Hi @gilbsgilbs

Don' worry about the code written, I wanted to understand how the plugin works, it was a good starting point!

Anyway, I still think we should adopt the new line at the end of the file by default (even if you decide not to have a config option) and this is not related with keeping or not the white spaces: if I generate a file first time, I think it should have new line at the end of the file.

JSON.stringify don't include this new line because it is not its concern: it is returning a string, not writing a file.

I see 2 paths:

  1. keep the POSIX standard and add the new line at the end of the file (and, in this case, I would update the PR to remove the option and include new line always).

  2. let this to the user fix it once (by adding the new line) and then keep white spaces and order.

But man, you are in vacation, let's discuss it when you are back 😄

Thanks for your time!

@gilbsgilbs
Copy link
Owner

Agreed with everything you said. I would go with the newline by default for new files. For existing files, trailing newlines and whitepaces should remain untouched. Users not wanting the new line could still remove it.

gilbsgilbs added a commit that referenced this pull request Sep 10, 2019
By default, JSON files created by the JSON exporter will also have a
trailing newline in order to be POSIX compliant. Though if the
whitespace is stripped at a later point in time by user, the exporter
will leave it as is.

Fixes #59
Closes #58
gilbsgilbs added a commit that referenced this pull request Sep 10, 2019
By default, JSON files created by the JSON exporter will also have a
trailing newline in order to be POSIX compliant. Though if the
whitespace is stripped at a later point in time by user, the exporter
will leave it as is.

See #59
Closes #58
gilbsgilbs added a commit that referenced this pull request Sep 10, 2019
By default, JSON files created by the JSON exporter will also have a
trailing newline in order to be POSIX compliant. Though if the
whitespace is stripped at a later point in time by user, the exporter
will leave it as is.

See #59
Closes #58
gilbsgilbs added a commit that referenced this pull request Sep 10, 2019
By default, JSON files created by the JSON exporter will also have a
trailing newline in order to be POSIX compliant. Though if the
whitespace is stripped at a later point in time by user, the exporter
will leave it as is.

See #59
Closes #58
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

2 participants