smhg / gettext-parser

Parse and compile gettext po and mo files, nothing more, nothing less
MIT License
160 stars 44 forks source link

Added escape characters option #67

Closed elitenas closed 2 years ago

elitenas commented 2 years ago

Added an option to skip escape newlines and quotes characters functionality.

Use case: We need the string to be exactly the same as what is in our database because when we import it back into our compiler, it can cause issues.

smhg commented 2 years ago

@thank you for your contribution!

I think it can be merged if you can please have a look at these:

elitenas commented 2 years ago

@smhg all done

smhg commented 2 years ago

@sinnstudio-nas thank you! Can you please have a look at why the tests fail?

elitenas commented 2 years ago

Fixed

smhg commented 2 years ago

Please put both changes in separate branches and create separate PR's. This makes it possible to see which changes and tests belong to what feature. Especially with the tests that is very important to have some level of stability for the next release. Thank you!

Edit: let me know if you need any help with that as git can be hard.

elitenas commented 2 years ago

@smhg all done. Please merge so I can make a pr for the folding changes :)

smhg commented 2 years ago

@sinnstudio-nas thank you for the changes. Tests are failing on Windows now. Can this be connected to this line you took out?

One more question: can you make the tests fixtures for this feature more concise? Please remove most of the double fixture content that is not related to escaping. Those parts that get tested already in other tests. It makes it easier to look at the tests at a later point in time.

elitenas commented 2 years ago

@smhg fixed... please try the merge again :)

smhg commented 2 years ago

Thank you! As per my comments: I think the test fixtures can still be significantly shorter. After that, we're good to merge.

elitenas commented 2 years ago

@smhg I simplified tests as much as I could

smhg commented 2 years ago

Possibly the Content-Type header has to stay. Others can be removed I think. Can you give that a try?

elitenas commented 2 years ago

@smhg done

elitenas commented 2 years ago

@smhg done

smhg commented 2 years ago

The PR looks good now, thank you for your efforts! The test is failing now however. Can you have a look at that still?

elitenas commented 2 years ago

I don't have access to my PC for a week due to vacation. I will take a look after. Thanks!

smhg commented 2 years ago

@sinnstudio-nas take your time - there is no deadline in open source ;)

elitenas commented 2 years ago

@smhg fixed. Please merge.