php-gettext / Gettext

PHP library to collect and manipulate gettext (.po, .mo, .php, .json, etc)
MIT License
687 stars 134 forks source link

Fix stripping backslash from new line characters in JS files (#200) #201

Closed jakubjo closed 5 years ago

jakubjo commented 5 years ago

This is the PR for previously created issue #200.

I'm not quite sure if its enough. Everything went fine in my tests with 756 strings accross 89 files.

jakubjo commented 5 years ago

... btw. Twig tests failed also in my local environment. Didn't looked after it, since it's not part of the PR.

jakubjo commented 5 years ago

Fixed it -- tests are running fine.

Screenshot 2019-03-25 00 29 53
jakubjo commented 5 years ago

Unfortunately we've encountered additional issues where js files are not parsed properly. I'll investigate further and come back with more informations later.

jakubjo commented 5 years ago

I've migrated the JS parser to use @mck89's peast parser.

The parser had issues with various, presumably legacy styled, files. Some strings would not be extracted. I've added a failing test first, so you're able to verify the bug by checking out commit bc85e89.

Alle tests are now green.

Additionally I've tested it on 285 different JS files ranging from legacy ugly pre ES2015 styled files to newest babel based styled files.

oscarotero commented 5 years ago

Sorry but I wouldn't like to add more dependencies to this package, specially if it's something required only by one extractor (and most users wont use).

If you want to use peast (and looks a great project), I recomend to create a different package for this extractor and use Gettext as dependency.

jakubjo commented 5 years ago

The elegance of your package lies in the all-in-one solution.

Why care about an additional dependency? Is there any significant issue with an additional dependency? That's the soul of open source: Don't reinvent the wheel.

and most users wont use

But essentially you don't know? The extractor is part of your package and has a bug in the current implementation. Which would be fixed with this PR.

But you're the package owner and you're here to decide: So final words were spoken?

oscarotero commented 5 years ago

The elegance of your package lies in the all-in-one solution.

To me, the elegance of this package is that it can be extended by the community with other features. For example, there are twig translators, symfony bundlers, laravel and cake integrations, wordpress, cli tools, etc. I love that people want to create and maintain these packages, but I wouldn't like to have it integrated in this package. The more dependencies and features, the more work and time is needed to maintain everything up to date.

The extractors provided do not require dependencies, because many people install this package only for the po->mo conversion, or any other specific feature, so I do not want to force them to install a package that they do not need, or even worse, may cause conflicts with their dependencies.

I made a commit to solve this issue. Let me know if it works fine now.

jakubjo commented 5 years ago

Thank you very much for your efforts. Its perfect! 👍

oscarotero commented 5 years ago

Great! I close this.