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

fix(cli): update translationIO service in CLI package (to handle context) #1949

Merged
merged 4 commits into from
Jun 17, 2024

Conversation

didier-84
Copy link
Contributor

Description

We noticed that our Translation.io service, in the CLI package, did not handle the new context attribute (introduced in v4 of Lingui). This PR updates the functions that convert PO items to our API segments and back, now handling this context attribute and thus resolving JS catalog issues.

This change ensures retro-compatibility for existing Translation.io users that already have an active Lingui project.

This PR has no impact for developers not using the Translation.io service.

N.B. we also updated the documentation page describing the Translation.io tool (missing image + new logo).

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Examples update

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation (if appropriate)

Copy link

vercel bot commented Jun 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 12, 2024 11:53am

Copy link

github-actions bot commented Jun 12, 2024

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.88 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.67 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

@timofei-iatsenko
Copy link
Collaborator

Guys, the whole translation.io functionality not covered by tests in the lingui monorepo, that is going against the code quality standards set for this project.

That's also why it was hard for me to update it when I worked on v4 version.
Honestly, I don't think this kind of integration should be a part of lingui monorepo at all. I would prefer to extract it nd have it separately maintained by translation.io crew or community.

The main problem right now it's partially copying functionality of other lingui modules, which makes it's particularly hard to maintain (you need to port changes in two places) and this maintenance burden is laying on especially my shoulders where I'm not interested in doing that.

@MichaelHoste
Copy link
Contributor

Hi @thekip ,

Thanks for your quick response and feedback about this!

At the time it seemed like a good idea to allow for an easy integration with translation providers. It's something that was discussed before implementing it and we decided to make it flexible enough for other providers to create their own integration.

We understand your position and we really don't want to add any extra burden on your shoulders, you maintain a great library and we don't want to be in the way! On the flip side, we have hundreds of active projects using this integration on a day-to-day basis that we really don't want to trouble.

Since this PR has no breaking change and just adds the context from V4, it would be nice for us is you accept to merge it.

But for the future, maybe we could find a way to work together and lift this integration from your shoulders? We could add some tests (by mocking the Translation.io response?) or assist you quickly when changes are needed.

Previously, we sponsored this project but the page disappeared when the team of maintainers changed. We would be happy to continue if a new initiative were to take place.

@andrii-bodnar andrii-bodnar changed the title Update translationIO service in CLI package (to handle context) fix(cli): update translationIO service in CLI package (to handle context) Jun 12, 2024
@timofei-iatsenko
Copy link
Collaborator

Let's do that in the following way:

  1. Merge this one
  2. Decide on the future plan of this integration.

I don't think that lingui monorepo is the right place for this, as i mentioned. I checked the sourcecode quickly and i believe this integration could be done as a separate cli command, and it would work as long lingui produce a correct *.po file.

So the usage might looks like following:

lingui extract && translation sync

I think this approach even more beneficial for you as a company since this cli tool will automatically support any existing libraries which use gettext po format.

Regarding existing integration, we can mark it as deprecated once you will be ready with your own tool and will provide a migration guide. It shouldn't be more complicated than just replacing one command with another.

@andrii-bodnar what do you think about that?

@andrii-bodnar
Copy link
Contributor

I agree, it seems like having a separate CLI for this will be better and open up new possibilities. The current approach with the service configuration is not flexible (for example, the service could have some additional configuration, not just the API key, and the current interface doesn't allow this. Also, the hardcoded API key in the Lingui configuration introduces a security issue that prevents the configuration from being committed to the repo). The Crowdin's approach of the integration with Lingui looks pretty stable and battle-tested.

Also, it feels like such an integration is not the responsibility of the i18n library.

Let's merge this PR and discuss the future of this integration and ways to make it better for Translation.io customers and the entire Lingui community.

Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 75.43%. Comparing base (d6b9698) to head (9ebaf65).
Report is 33 commits behind head on main.

Files Patch % Lines
packages/cli/src/services/translationIO.ts 0.00% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1949      +/-   ##
==========================================
- Coverage   76.65%   75.43%   -1.22%     
==========================================
  Files          81       85       +4     
  Lines        2090     2133      +43     
  Branches      533      545      +12     
==========================================
+ Hits         1602     1609       +7     
- Misses        375      411      +36     
  Partials      113      113              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MichaelHoste
Copy link
Contributor

Thanks you for your feedback @thekip and @andrii-bodnar.

We tend to agree with you with this approach.

The most important for us is that Lingui V4 continues to work properly with Translation.io without any breaking changes.

For the future (v5?) we agree with you that the creation of a separate CLI would be the correct approach. We are already in the process of creating something like that to manage any type of translation project and file types. Lingui will therefore be our first test case.

If it's okay with you, when the time comes, we will simply update the documentation to provide a path for those who will need to migrate to the CLI.

Thank you very much for your understanding and for merging this PR!

@andrii-bodnar andrii-bodnar merged commit ea7b9e7 into lingui:main Jun 17, 2024
15 of 16 checks passed
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.

4 participants