Added \r to \Gettext\Generators\Po::convertString()

[vaites]

First step to fix #218

[oscarotero]

Great! I miss the normalizeLineBreak option (or similar name) and some tests.

[vaites]

Yes, I'm working on it, just added it as a first step to solve this problem. El mié., 7 ago. 2019 11:25, Oscar Otero <[email protected]> escribió:

…

Great! I miss the normalizeLineBreak option (or similar name) and some tests. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#219>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADU3RFLTUAXOHKGLD4IYH3QDKIITANCNFSM4IJ2XD4A> .

[oscarotero]

@vaites When do you think I can merge this? I'm about to release a new version and would like to include this fix.

[vaites]

Sorry, I'm very busy now. Release the new version without it...

[vaites]

After some doubts, here are the changes:

1. Added \r to \Gettext\Generators\Po::convertString()
2. Added line break normalization into \Gettext\Translations
3. Added sortOutput option to \Gettext\Generators\Po

My doubt was where to add the filter with minimum changes. I added first to Generator::toFile() but Generator::toString() doesn't was filtered, so I added the filter() method to Translations class. I didn't added the normalization to Translation::setTranslation() because I think we mustn't fix anything at load, just explicitly when saving.

Feel free to rename methods or directly apply it on Translation::setTranslation() if you want.

[oscarotero]

Ok, thanks for the contribution. I have some comments:

• My intention here was add this feature without change the Translations/Translation classes. Sorry, I should be more clear before. This is about fixing or normalizing how translations are extracted/generated. I think is better to include this option in the extraction and/or generation time. For example:

$translations = Translations::fromPoFile('translations.po', ['normalizeLineBreaks' => true]);

• This can be done using a function in Utils that can be used by all extractors and generators.

• The setNormalizeLineBreaks and getNormalizeLineBreaks are dynamic methods modifying static variables. This is usually a bad idea. And the name method filter() is confusing. Anyway, I recomend to do not modify these clases and add this functionality to the generators and extractors.

• Sort options is a great idea, but I'd rather having a pull request for each new feature.

[vaites]

Just those were my doubts. I thought about applying the change to the generators but I thought it would be more appropriate to add a generic method (filter) to add there future filters.

No problem. Will add it to the generators (I think extractors must not do this) and will make a pull request for each feature. Do you want to be added to all generators, right?

[oscarotero]

The problem I see is that translations/translation do not have other methods to fix issues like this (not only line-breaking but also unicode characters, etc). All that stuff is fixed currently in the extractors or generators, so I think is better (IMHO) to be consistent with this, and the translation instances are ready to consume without need to do additional fixes.

Do you want to be added to all generators, right?

This is an option. But why not in the extractors instead generators?

Thanks!

[vaites]

My option was to add only to the generators so as not to affect the loading of existing files at any time. If the user wants to normalize, must set it at save.

If you set it only on generators, you can't normalize if you create and save a collection, so this must be added to both extractors and generators.

If you don't see any problem with the normalization at loading, why not add this filter to Translation::setTranslation() and Translation::setPlural()?. Extractors use this methods to set the translations, and any translation created by the user too. With this method nothing is modified except Translation class and an option on Translations class to set on load/save...

[oscarotero]

Ok, after reading again all issues, I think I understand now the whole problem.
We could skip the line breaking normalization for now and just fix the problem with PO files. Doing so, we keep the content unaltered but providing a better windows support for PO.

What do you think?

[vaites]

I understand that you only want to escape \r character and revert all other changes, right?

I will send you a separated pull request to sortOutput option.

[oscarotero]

Yes, sorry for the continuous changes 🙏

[vaites]

OK, no problem. In understand this issue is bigger than it seemed at first.

[vaites]

Well, let's discard this pull request and make a new one forked from the previous commit, instead of reverting all the changes made from there...