search

potsky / laravel-localization-helpers

🎌 Artisan commands to generate and update lang files automatically
GNU General Public License v3.0
187 stars 38 forks source link

Fixed multi-line regex #35

Closed Max13 closed 7 years ago

Max13 commented 8 years ago
Max13 commented 8 years ago

I think, if accepted, should be pushed to laravel 4 and laravel 5 branches. As it fixes the issue for both versions

daverdalas commented 8 years ago

I guess your version of regex formula does not work for all cases :( https://regex101.com/r/cT6hB2/2

Max13 commented 8 years ago

I can’t get it to separate arguments :’(

This is my best for now (found on SO): https://regex101.com/r/cT6hB2/3

Max13 commented 8 years ago

@daverdalas I literally spent the day on it. It's almost impossible without specifying every parameters 1 by 1 (and not using a quantifier on a group), that's because the parameters are found by comma, and in an array, we also find commas. I'm getting lost, what do you exactly match today?

Do you want to match then trim? Do you want to perfectly match without surrouding whitespaces? Or do you just match with or without commas, you don't currently care in your code?

Here is where I am: https://regex101.com/r/cT6hB2/4

Max13 commented 8 years ago

@daverdalas Here you are ! https://regex101.com/r/cT6hB2/5

Finally, for the great final... This last version handles function names as arguments, texts AND constants: https://regex101.com/r/cT6hB2/6

daverdalas commented 8 years ago

@Max13 thank you for your efforts but I think that there is a simpler solution. Instead of trying to handle multi-lines in regex just let's remove them :) See https://github.com/potsky/laravel-localization-helpers/pull/36

Max13 commented 8 years ago

😟Well... Ok then, seems better.

potsky commented 8 years ago

Hi !

I will be back in 2 weeks to take a look on your PR. It seems your PR makes all travis CI checks fail due to a Bing translator problem. So I cannot merge it without taking a look on the code.

Max13 commented 8 years ago

@potsky Hi, I saw that but thought it was not related to my commits. I think I ran the tests before my commits, but don't remember if it still failed.

Anyway, take your time, but as @daverdalas wrote, maybe #36 is a better solution ?

potsky commented 7 years ago

Hi,

in fact his solution is the correct way to handle multiline regex. The only drawback is that people who has already written multline regex configuration, will be obliged to rollback to a flat configuration. But it should concern a very few people...

I will keep your tests because @daverdalas has not :-)