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

feat: msgctxt support #1094

Merged
merged 3 commits into from
Oct 18, 2021
Merged

feat: msgctxt support #1094

merged 3 commits into from
Oct 18, 2021

Conversation

nakkamarra
Copy link
Contributor

As described as part of: #856

My decision here was to use context / msgctxt as a way to "sub-key" a given key, which I feel lines up more with the original gettext implmentation/intention of context. So if we had a catalog like so:

{
    "keyA": "translationA",
    "keyB": "translationB",
}

the addition of context could be modeled like so:

{
    "keyA": "translationA",
    "keyB": "translationB",
    "contextA": {
         "keyA": "translationAA",
         "keyB": "translationAB"
     },
     "contextB": {
         "keyA": "translationBA"
     }
}

where a given context string acts as the key of another sub-catalog of translations. This allows one to add multiple translations for a given ID / string by using different contexts. To pull out translations, one would reference this.messages[context][id] instead of this.messages[id] in the case that context was defined for a given ID.

On top of that, I think this is actually pretty trivial to support for many of lingui's current formats (po, po-gettext, minimal, and raw), since we're just creating sub-objects and serializing / deserializing them. As far as CSV is concerned, I think there'd need to be a bit of extra work, but it could be possible to apply something similar to what we're doing with JSON (imagine instead of each row being key, translation you have context, key, translation and work with that)

There's the caveat that this does not abstract msgids and solve the shrinking of IDs in catalogs, one of the upsides to @Bertg's approach to this problem.

I'd say this is about 90% of the way there, but I'd love to get some feedback and thoughts around this approach, particularly around this chunk which could be refactored to be much cleaner: https://github.com/lingui/js-lingui/compare/main...nakkamarra:msgctx-support?expand=1#diff-9317c05f72203559bc3788fb80d76d19f38b457e50c35c430463d964574eec90R27-R80

@semoal @tricoder42 @Bertg

@vercel
Copy link

vercel bot commented Jun 30, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/lingui-js/js-lingui/Fo95hJ74iAUVNaZpH8Hm7G7nGaCz
✅ Preview: https://js-lingui-git-fork-nakkamarra-msgctx-support-lingui-js.vercel.app

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #1094 (fbdd6a0) into main (54d6b6e) will decrease coverage by 0.38%.
The diff coverage is 70.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1094      +/-   ##
==========================================
- Coverage   81.88%   81.49%   -0.39%     
==========================================
  Files          56       56              
  Lines        1678     1724      +46     
  Branches      458      474      +16     
==========================================
+ Hits         1374     1405      +31     
- Misses        182      189       +7     
- Partials      122      130       +8     
Impacted Files Coverage Δ
packages/cli/src/api/catalog.ts 81.48% <ø> (-0.09%) ⬇️
packages/react/src/Trans.tsx 78.57% <ø> (ø)
packages/cli/src/api/formats/po-gettext.ts 82.40% <33.33%> (-1.21%) ⬇️
packages/cli/src/api/formats/po.ts 95.74% <33.33%> (-4.26%) ⬇️
packages/cli/src/api/compile.ts 87.93% <61.53%> (-8.07%) ⬇️
packages/core/src/i18n.ts 71.42% <66.66%> (-4.02%) ⬇️
...ackages/babel-plugin-extract-messages/src/index.ts 79.05% <76.31%> (-3.35%) ⬇️
packages/macro/src/constants.ts 100.00% <100.00%> (ø)
packages/macro/src/macroJsx.ts 91.17% <100.00%> (+0.19%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca4978f...fbdd6a0. Read the comment docs.

@nakkamarra
Copy link
Contributor Author

Hey @semoal think you could take a look when you have the chance?

@jeroenvisser101
Copy link
Contributor

@nakkamarra this looks great! @semoal is this something that could make it in a future release? We have a few strings in our application that we have issues with and our current workaround (custom msgids) isn't the greatest way to handle this. Let me know if we can help :)

@nakkamarra
Copy link
Contributor Author

@jeroenvisser101 thanks, I'm no longer working with lingui and this PR is probably a bit outdated now, but if you wanted to grab this branch and merge the main branch and see if there are any conflicts / changes that need to be made I think it'd be a good way to see this feature make it over the finish line.

@semoal semoal merged commit 8ee42cb into lingui:main Oct 18, 2021
@semoal
Copy link
Contributor

semoal commented Oct 18, 2021

It's cool @nakkamarra I reviewed it and look fine to me, let's release a new version tomorrow with this feature and if @jeroenvisser101 finds some issues feel free to open an issue and I'll solve them asap :)

@semoal
Copy link
Contributor

semoal commented Nov 26, 2021

After so much time, finally this is released guys.. Sorry for the delay I've been lately so busy ..
3.13.0

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.

3 participants