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

Ensure logic that lints localizations' string length considers both LF and CRLF as one character #382

Closed
mokagio opened this issue Jul 12, 2022 · 2 comments · Fixed by #383
Assignees
Labels
bug Something isn't working

Comments

@mokagio
Copy link
Contributor

mokagio commented Jul 12, 2022

We recently run into one translation flagged as too long but only because it used CRLF.

As @AliSoftware wrote in an internal conversation (ref pxLjZ-7aA-p2#comment-19225):

[The tooling] should count newlines as only one character even if they are represented as CRLF (providing the correct explicit IO flags and encoding on the File.open might be enough?)

@mokagio mokagio changed the title Ensures logic that lints localizations' string length considers both LF and CRLF as one character Ensure logic that lints localizations' string length considers both LF and CRLF as one character Jul 12, 2022
@mokagio mokagio added the bug Something isn't working label Jul 12, 2022
@mokagio mokagio self-assigned this Jul 12, 2022
@AliSoftware
Copy link
Contributor

Actually I wonder if the issue might not be CRLF being 2 characters, but the fact that we determine the length reading from the JSON export that we then manipulate manually a bit, and maybe that JSON Ruby string contains \n literals, counting \ and n as two separate characters.

I'm not sure about that because I don't understand why JSON.parse wouldn't transform those two \n character from the raw JSON data into a newline interpreted character after parsing. But also, that comment about … - 4 # Don't count JSON delimiters scares me a little and makes me wonder if we don't have raw JSON at that stage (otherwise why would that comment mention JSON delimiters?)

Anyway, all this code from metadata_download_helper as always been quite messy and with a convoluted implementation so it could also be the occasion to clean it up a bit to make it more readable and less convoluted. (Alternatively, we could directly go with the route of fixing all the issues with the API and implement the new approach/API from paaHJt-31O-p2, but that would be much more involved work, while maybe we'll need a quick fix for the time being)

@AliSoftware
Copy link
Contributor

AliSoftware commented Jul 12, 2022

Ok, gave it a try after all, and that was indeed one more oddity of that mess of a code that is metadata_download_helper (and the JSON export format provided by GlotPress) rather than an issue with CRLF.
This PR should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants