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

[WIP] Throws exceptions when variable expressions mismatch #84

Merged
merged 5 commits into from Jun 3, 2017

Conversation

Projects
None yet
2 participants
@MrOrz
Contributor

MrOrz commented May 15, 2017

Tacking with #83 .

Several issues should take into consideration (but is not done yet):

  • merge assertExpressionExists with msgid2Orig
  • Should skip checking if replaceVariablesNames is false, or we should add an option to toggle the check - won't fix. replaceVariableNames will be removed in the future.
Johnson Liang
@AlexMost

This comment has been minimized.

Member

AlexMost commented May 15, 2017

Great job. Will save a considerable amount of time while fixing bugs related to translator typos.

Discuss if the implementation should directly alter msgid2Orig (The newly implemented assertExpressionExists have similar signatures and identical occurrences)

I think it makes sense, because of the same occurrences.

Should skip checking if replaceVariablesNames is false, or we should add an option to toggle the check

As far as I see babel-plugin works only with ${ varName } format. And replaceVariablesNames setting is available only for the library. I guess we will remove that setting on the next release.

@MrOrz

This comment has been minimized.

Contributor

MrOrz commented May 16, 2017

@AlexMost thanks for your advice! TODO list is updated.

Johnson Liang added some commits May 16, 2017

@codecov

This comment has been minimized.

codecov bot commented May 16, 2017

Codecov Report

Merging #84 into master will increase coverage by 0.14%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #84      +/-   ##
=========================================
+ Coverage   96.45%   96.6%   +0.14%     
=========================================
  Files          13      13              
  Lines        1045    1059      +14     
  Branches      151     152       +1     
=========================================
+ Hits         1008    1023      +15     
+ Misses         37      36       -1
Impacted Files Coverage Δ
src/utils.js 95.02% <100%> (+0.1%) ⬆️
src/errors.js 86.36% <85.71%> (+4.01%) ⬆️
src/resolve.js 100% <0%> (+3.12%) ⬆️

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 9e249ec...a29ebd5. Read the comment docs.

expect(func).to.throw(
'unknown: Expression \'mississipi\' is not found in the localized string \'Typo test ${ missingpi }\'.'
);
});

This comment has been minimized.

@MrOrz

MrOrz May 22, 2017

Contributor

@AlexMost Since the expression checking mechanism is now built-in in msgid2Orig, do we still need these functional tests?

This comment has been minimized.

@AlexMost

AlexMost May 22, 2017

Member

Seems like we don't, but as for me, I would leave them. So it's up to you.

const input = '${0 } banana ${ 1}';
const expected = '`${ a } banana ${ b }`';
expect(msgid2Orig(input, ['a', 'b'])).to.eql(expected);
});
it('should ignore white spaces inside expressions', () => {
it.skip('should ignore white spaces inside expressions', () => {

This comment has been minimized.

@MrOrz

MrOrz May 22, 2017

Contributor

@AlexMost Here I skipped 2 tests that is tackling the legacy ${0}. Checking existence of expressions against the translated strings will definitely break these tests. Should we remove these tests?

This comment has been minimized.

@AlexMost

AlexMost May 22, 2017

Member

As far as I see those tests are changed already in master.

@MrOrz

This comment has been minimized.

Contributor

MrOrz commented May 22, 2017

sorry, did not notice that my line comments were hold back in un-submitted code reviews ._.

@AlexMost

This comment has been minimized.

Member

AlexMost commented May 24, 2017

@MrOrz do you need some help, here? I guess except resolving the conflicts everything is ready for merging.

@MrOrz

This comment has been minimized.

Contributor

MrOrz commented May 24, 2017

@MrOrz

This comment has been minimized.

Contributor

MrOrz commented May 25, 2017

@AlexMost conflict resolved. However, since I have disabled tests about supporting ${ 0 }, it seems that the coverage has dropped :/

@AlexMost

This comment has been minimized.

Member

AlexMost commented May 25, 2017

@MrOrz nice! No worries about the coverage.

@AlexMost AlexMost merged commit 4975f1f into ttag-org:master Jun 3, 2017

2 of 3 checks passed

codecov/patch 92.85% of diff hit (target 96.45%)
Details
codecov/project 96.6% (+0.14%) compared to 9e249ec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@AlexMost

This comment has been minimized.

Member

AlexMost commented Jun 3, 2017

Thanks a lot for your work! I will take care of those skipped tests.

@AlexMost

This comment has been minimized.

Member

AlexMost commented Jun 6, 2017

skipped tests are fixed and published in 0.5.6 ver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment