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

vaites commented 5 years ago

First step to fix #218

oscarotero commented 5 years ago

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

vaites commented 5 years ago

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 notifications@github.com 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 https://github.com/oscarotero/Gettext/pull/219?email_source=notifications&email_token=AADU3RFYXYCG3PHDBBMXT3LQDKIITA5CNFSM4IJ2XD4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3XZLIY#issuecomment-519017891, or mute the thread https://github.com/notifications/unsubscribe-auth/AADU3RFLTUAXOHKGLD4IYH3QDKIITANCNFSM4IJ2XD4A .

oscarotero commented 4 years ago

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

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

vaites commented 4 years ago

After some doubts, here are the changes:

Added \r to \Gettext\Generators\Po::convertString()
Added line break normalization into \Gettext\Translations
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 commented 4 years ago

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

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

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

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

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

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

Yes, sorry for the continuous changes 🙏

vaites commented 4 years ago

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

vaites commented 4 years ago

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...

php-gettext / Gettext

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