search

boxblinkracer / phpunuhi

PHPUnuhi - The easy composable framework to validate and manage translations
MIT License
72 stars 5 forks source link

Change binary detection to also support umlauts and other special cha… #50

Closed mjosef89 closed 1 month ago

mjosef89 commented 1 month ago

The previous implementation flags non-ASCII characters as binary, such as umlauts. The new solution takes a different approach to recognizing whether it is a binary string.

Source: https://stackoverflow.com/a/76816100

boxblinkracer commented 1 month ago

hey there, thats super cool i didnt know that so basically i would love to merge it, could you maybe add a unit test to it? there are some existing, and it would be great to see how the change makes a new test green and the changelog would also be nice to add

but i would also merge it without additional changes on your side just if you would be happy to do so :)

mjosef89 commented 1 month ago

Hi, I have now extended the existing test and added a changelog entry.

boxblinkracer commented 1 month ago

hi @mjosef89 thats perfect thank you so much, so I think I know get the impact, because you were just adding negative tests for umlaute, that means that strings with umlaute were accidentally recognized as umlaute TRUE? is that correct? ....because unit tests tell a story haha

if you could fix the pipe, i would merge it

also, there is a "make pr" command, if you run that in your dev-environment, that would ensure that everything is green so it starts the fixer (you might need to commit this again in your git) and run all tests for you

mjosef89 commented 1 month ago

Hi @boxblinkracer,

so I think I know get the impact, because you were just adding negative tests for umlaute, that means that strings with umlaute were accidentally recognized as umlaute TRUE? is that correct? ....because unit tests tell a story haha

Strings containing umlauts were previously recognized as binary, so the isBinary() function would return true in this case.

I added a few strings, some with umlauts/Non-ASCII chars to test that the method now always returns false in these cases, so they are indeed not binary.

I hope that fits, otherwise please let me know what I should change.

also, there is a "make pr" command, if you run that in your dev-environment, that would ensure that everything is green so it starts the fixer (you might need to commit this again in your git) and run all tests for you

Sorry, I didn't know that, but it's done now too

boxblinkracer commented 1 month ago

hi there awesome its all perfect now theres a test, theres a clear explanation, great code and thanks for fixing the pipe

no worries on the "make pr" ill add it to a contribution guide

so all that is left now is -> merge :)