Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use file paths when converting .json to .po #56

Closed
wants to merge 1 commit into from
Closed

Use file paths when converting .json to .po #56

wants to merge 1 commit into from

Conversation

emmagamma
Copy link

@emmagamma emmagamma commented Oct 28, 2016

This PR adds the capability, when converting from .json to .po, to add comments to the .po file with the file path information for where strings were found. This special format of the i18next .json (as described in the PR which implements it) can be optionally output by the i18next-parser by including --track-paths true. Converting from the original .json format should remain unaffected, as these changes make it possible to mix formats dynamically, adding file path comments when available and leaving them out when not available.

fixes #55

@coveralls
Copy link

coveralls commented Oct 28, 2016

Coverage Status

Coverage decreased (-1.05%) to 95.359% when pulling c4d6831 on sourceRS:detect-and-use-file-paths-on-json-to-po into c8f5f99 on i18next:master.

@emmagamma
Copy link
Author

whoops, will try and improve that coverage.

@coveralls
Copy link

coveralls commented Dec 30, 2016

Coverage Status

Coverage increased (+1.0%) to 97.38% when pulling 75c830d on sourceRS:detect-and-use-file-paths-on-json-to-po into c8f5f99 on i18next:master.

if (options.keyasareference) {
if (typeof trans[kv.context][kv.value] === 'object') {
// same context and msgid. this could theorically be merged.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this conditional isn't needed anymore? So I went ahead and removed it, which helped increase the test coverage - since the same kv.key is going to the same place in both branches of the conditional, and since the tests don't ever hit the positive case. But if the right solution should actually just be to add tests that do hit this case, definitely let me know, I'd imagine this might potentially be useful for someone? it's just not clear to me how exactly 🤷‍♀️

@perrin4869 perrin4869 self-assigned this Mar 6, 2017
@libbymo
Copy link

libbymo commented Mar 13, 2017

@perrin4869 Was wondering about the status of this and i18next/i18next-parser#56. Is there anything further we can do to get this merged?

@perrin4869
Copy link
Contributor

@sourceRS @libbymo sorry for the delay! Promise to take a look at this this weekend :D

@emmagamma
Copy link
Author

Oh rad @perrin4869 thanks so much!! and feel free to let us know if anything needs updating before you're comfortable merging.

@perrin4869
Copy link
Contributor

@sourceRS Hey, sorry for the long wait! Looking more into this issue, I think maybe it's a better idea to wait for i18next/i18next-parser#56 to be merged first before going ahead with this PR. I think this PR really only makes sense if i18next-parser supports the proposed format here to begin with... Is that fair to say?

@emmagamma
Copy link
Author

Oh yes, totally @perrin4869 ~ aside from that though, are you good with the changes here?

@perrin4869
Copy link
Contributor

Well, it would be cool to also have the reverse, .po -> .json conversion supported too, but yeah the changes here look awesome!

@emmagamma
Copy link
Author

emmagamma commented Apr 4, 2017

My thinking was just that this is intended to allow translators to gain more context by showing them where in a given project to find the strings and see how they're being used. So since translators tend to work in Transifex and similar tools which integrate with .po files, and given that the work in the other PR generates a .json file in the desired format already, I figured that it might be better to keep it simpler and not add .po -> .json functionality unless that would help support a useful workflow for people using this library?

I don't think it'd be difficult to add, but it just didn't seem useful I guess. I'm happy to defer to you if you think people would benefit from it.

@jamuhl
Copy link
Member

jamuhl commented May 8, 2018

@sourceRS @perrin4869 not sure about the status of this...i18next/i18next-parser#56 was closed out of bad luck i guess -> 1.0.0_beta release of parser?!?

@libbymo
Copy link

libbymo commented May 9, 2018

Yes, I'm guessing this should probably be closed. We haven't begun any work around retargeting this for the 1.0.0_beta release.

@perrin4869 perrin4869 closed this Feb 3, 2019
@perrin4869
Copy link
Contributor

closing due to i18next/i18next-parser#56

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.

Tracking file path references for each key
5 participants