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

Enable pseudolocalization from pseudolocale folder #836

Merged

Conversation

MartinCerny-awin
Copy link
Contributor

@MartinCerny-awin MartinCerny-awin commented Nov 7, 2020

This PR utilises standard getTranslation eg. we first try to get translation from locale folder, then from fallback, and then keys, etc...

This will allow us to set text that is going to be pseudolocalized and it will close #744

I do not think there is a reason to change to pseudolocalizing on extract, but it will solve the issue @nathanforce is having.

@vercel
Copy link

vercel bot commented Nov 7, 2020

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/h9c3mabfw
✅ Preview: https://js-lingui-git-simplify-pseudolocale-compile.lingui-js.vercel.app

The pseudolocalization is automatically created from default messages.
PseudoLocale string have to be in `locale` config as well. Otherwise no folder is going to be created.
Pseudolocalized text is created on ``yarn compile`` command.
The pseudolocalization is automatically created from TODOTODOTODO.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have already in documentation what is the prioritisation of compiling messages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. It's:

  1. translation
  2. fallback (using the new fallbackLocales)
  3. message key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it be worth adding it to documentation? I just can't figure out where it could fit

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, absolutely. Somewhere inside compile docs? Because it ultimately happens at compile step.

@codecov
Copy link

codecov bot commented Nov 7, 2020

Codecov Report

Merging #836 (73a23fe) into main (3924d79) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #836    +/-   ##
========================================
  Coverage   83.74%   83.75%            
========================================
  Files          39       51    +12     
  Lines        1335     1625   +290     
  Branches      357      457   +100     
========================================
+ Hits         1118     1361   +243     
- Misses        129      158    +29     
- Partials       88      106    +18     
Impacted Files Coverage Δ
packages/cli/src/api/formats/index.ts 55.55% <0.00%> (-4.45%) ⬇️
packages/cli/src/api/compile.ts 100.00% <0.00%> (ø)
packages/detect-locale/src/index.ts 66.66% <0.00%> (ø)
...kages/detect-locale/src/detectors/fromSubdomain.ts 40.00% <0.00%> (ø)
...ackages/detect-locale/src/detectors/fromHtmlTag.ts 25.00% <0.00%> (ø)
packages/detect-locale/src/detectors/fromPath.ts 40.00% <0.00%> (ø)
...ackages/detect-locale/src/detectors/fromStorage.ts 100.00% <0.00%> (ø)
packages/detect-locale/src/utils/query-string.ts 78.57% <0.00%> (ø)
packages/detect-locale/src/detectors/fromUrl.ts 75.00% <0.00%> (ø)
...kages/detect-locale/src/detectors/fromNavigator.ts 66.66% <0.00%> (ø)
... and 9 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 3924d79...3c53c19. Read the comment docs.

@Bertg
Copy link
Contributor

Bertg commented Nov 8, 2020

Would these changes resolve #838 ?

Copy link
Contributor

@tricoder42 tricoder42 left a comment

Choose a reason for hiding this comment

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

What's the status of this PR? I see you've removed some code, but I don't see anything that replaces it. That code was just obsolete?

The pseudolocalization is automatically created from default messages.
PseudoLocale string have to be in `locale` config as well. Otherwise no folder is going to be created.
Pseudolocalized text is created on ``yarn compile`` command.
The pseudolocalization is automatically created from TODOTODOTODO.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, absolutely. Somewhere inside compile docs? Because it ultimately happens at compile step.

Copy link
Contributor Author

@MartinCerny-awin MartinCerny-awin left a comment

Choose a reason for hiding this comment

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

I will improve documentation and then it can be merged

packages/cli/src/lingui-compile.ts Show resolved Hide resolved
Co-authored-by: Tomáš Ehrlich <tomas.ehrlich@gmail.com>
docs/ref/conf.rst Outdated Show resolved Hide resolved
Co-authored-by: Tomáš Ehrlich <tomas.ehrlich@gmail.com>
Copy link
Contributor

@tricoder42 tricoder42 left a comment

Choose a reason for hiding this comment

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

Looking good! I've just did a last check and found two small typos. Could you please check?

docs/tutorials/cli.rst Outdated Show resolved Hide resolved
docs/tutorials/cli.rst Outdated Show resolved Hide resolved
Co-authored-by: Tomáš Ehrlich <tomas.ehrlich@gmail.com>
@MartinCerny-awin
Copy link
Contributor Author

I am happy with the changes. I have tested them and it looks to be working for me.

@tricoder42 tricoder42 merged commit f1e0078 into lingui:main Nov 10, 2020
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.

[PseudoLocalization] Pseudolocalize on Extract
4 participants