Skip to content

Cstring escapes#12

Merged
rubenv merged 3 commits intomasterfrom
unknown repository
Jul 18, 2014
Merged

Cstring escapes#12
rubenv merged 3 commits intomasterfrom
unknown repository

Conversation

@johnyb
Copy link
Copy Markdown
Collaborator

@johnyb johnyb commented Jun 23, 2014

This should fix #2. Please don't merge, yet, still working on this branch. Just wanted to publish, what I already got.

ToDo

@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Jun 23, 2014

Cool, let me know when it's ready to merge.

johnyb and others added 3 commits June 23, 2014 15:03
in Item.toString, all \n characters are removed from the output.
The gettext tools however leave those characters intact. This
will now produce the same output as tools like msgcat.
Signed-off-by: Julian Bäume <julian@svg4all.de>

Conflicts:
	lib/po.js
writing messages should no be in line with gettext tools. I tested
using msgcat, it provides the same results.

For some common use-cases I wrote explicit tests, for uncommon and
even unwanted use-cases I wrote one test to make sure pofile works
like msgcat for those messages
@johnyb
Copy link
Copy Markdown
Collaborator Author

johnyb commented Jun 23, 2014

also rebased to latest master

@johnyb
Copy link
Copy Markdown
Collaborator Author

johnyb commented Jun 23, 2014

Ready to merge, once travis is green. Also fixes #2.

@johnyb
Copy link
Copy Markdown
Collaborator Author

johnyb commented Jun 23, 2014

I tried to comment my research in the code and hope it's understandable. I mostly tested how msgcat from gettext tools behaves and adjusted the tests accordingly. Then I changed implementation until tests were green.

Anyway, when releasing this, you might want to bump this as "incompatible", since handling of \n in msgid changed. It now works like xgettext would work.

@johnyb
Copy link
Copy Markdown
Collaborator Author

johnyb commented Jun 23, 2014

One more thing to add:

Parsing this po code

msgid ""
"something\n"
"else"
msgstr ""

msgid ""
"something"
"else"
msgstr ""

and writing it to a file, would produce an invalid po file with v0.2.11 and earlier. This PR would fix this issue, so this might indeed be considered a bugfix. d8fc514 contains the fix.

@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Jul 18, 2014

Oh darn, I missed this. This is still relevant, right?

Code looks good to me, can be merged unless you have a reason why it shouldn't be.

@johnyb
Copy link
Copy Markdown
Collaborator Author

johnyb commented Jul 18, 2014

Ahh, missed that myself, too :) Yes, IMHO this should still be in master and released as bugfix.

@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Jul 18, 2014

Nod. Rolling out release.

rubenv added a commit that referenced this pull request Jul 18, 2014
@rubenv rubenv merged commit e42dc28 into rubenv:master Jul 18, 2014
rubenv added a commit that referenced this pull request Jul 18, 2014
@rubenv
Copy link
Copy Markdown
Owner

rubenv commented Jul 18, 2014

Released to NPM as 0.2.12.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port escaping fixes

3 participants