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

#296: Add pseudolocalization #309

Merged
merged 8 commits into from
Sep 7, 2018
Merged

#296: Add pseudolocalization #309

merged 8 commits into from
Sep 7, 2018

Conversation

MartinCerny-awin
Copy link
Contributor

@MartinCerny-awin MartinCerny-awin commented Sep 1, 2018

Closes #296

Documentation
It is without documentation. I will add it later after we discuss that the code is ok.

Pseudolo Library
Newly published library https://www.npmjs.com/package/pseudolocale forked from pseudoloc with idempotence and option to specify delimiter using Regex special characters.

I have tried to use pseudoloc https://github.com/bunkat/pseudoloc library, but there there two behaviours which made it impossible. The first one was that the library was returning different results every time. This would cause changes in versioning system for unchanged translations. The second one was incompatibility with RegEx special characters. This made escaping HTML tags impossible.

I have also tried to use another libraries, but it required RegExp to match non HTML tags. I could not find the solution and SO did not have understanding for my question https://stackoverflow.com/questions/52121600/regular-expression-to-match-non-html-tags

At the end, I have fixed these problems in my fork: https://github.com/MartinCerny-awin/pseudoloc/commits/master

I can try to merge it into original library, but it doesn't look very active and it might take time or it might not even get merged as the author could have different opinion on idempotence. Should I create a new library while waiting for the merge?

Flow
I had to remove flow from packages/cli/src/lingui-extract.test.js as I could not type mocks.

Tests
Are there more tests to verify that pseudolocalization works correctly?

packages/cli/src/api/catalog.js Show resolved Hide resolved
packages/cli/src/api/compile.js Show resolved Hide resolved
packages/cli/src/api/pseudoLocalize.js Outdated Show resolved Hide resolved
packages/cli/src/api/pseudoLocalize.js Outdated Show resolved Hide resolved
}

function pseudoLocalize(message) {
pseudoloc.option.delimiter = delimiter
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these properties need to be specified again for each message? Is there some global state that needs to be reset every time? I haven't studied pseudoloc so closely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not needed every time. I will try to do it only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it. It is run only once with boolean switch. I didn't want to move setting the option outside of pseudoLocalize file

packages/cli/src/lingui-extract.test.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 2, 2018

Codecov Report

Merging #309 into master will increase coverage by 3.71%.
The diff coverage is 96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
+ Coverage   91.66%   95.38%   +3.71%     
==========================================
  Files          42       43       +1     
  Lines        1104     1127      +23     
==========================================
+ Hits         1012     1075      +63     
+ Misses         92       52      -40
Impacted Files Coverage Δ
packages/conf/src/index.js 96.55% <ø> (ø) ⬆️
packages/cli/src/api/pseudoLocalize.js 100% <100%> (ø)
packages/cli/src/api/catalog.js 94.11% <100%> (ø) ⬆️
packages/cli/src/lingui-extract.js 68.75% <100%> (+53.99%) ⬆️
packages/cli/src/api/compile.js 98.07% <66.66%> (-1.93%) ⬇️
packages/cli/src/api/stats.js 100% <0%> (+100%) ⬆️

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 1832213...6a59c4f. Read the comment docs.

@tricoder42
Copy link
Contributor

@MartinCerny-awin Looks pretty good! Is it ready for merge?

@MartinCerny-awin
Copy link
Contributor Author

I did documentation in my IDE. It might be good to check it. Otherwise I am happy with it.

@danielkcz
Copy link
Contributor

@MartinCerny-awin There are conflicts. Can you fix those, please?

@tricoder42
Copy link
Contributor

@FredyC I've just merged it. It was probably caused when I removed opencollective...

@tricoder42 tricoder42 merged commit 5c6a67d into lingui:master Sep 7, 2018
@tricoder42
Copy link
Contributor

@MartinCerny-awin Thank you! There're few minor formatting issues in docs, but I'll fix them on master.

Great work! Thank you for working on this 👏

@tricoder42 tricoder42 changed the title [WIP] #296: Add pseudolocalization #296: Add pseudolocalization Sep 10, 2018
Photonios pushed a commit to SectorLabs/js-lingui that referenced this pull request Dec 9, 2019
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
3 participants