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

Add resetDefaultValueLocale option #451

Merged
merged 1 commit into from
Nov 14, 2021

Conversation

lars-mortenson
Copy link
Contributor

Why am I submitting this PR

Add a feature related to resetting translations for a key if the default value changes.

Does it fix an existing ticket?

Yes #267

Checklist

  • [ X] only relevant code is changed (make a diff before you submit the PR)
  • [ X] tests are included and pass: yarn test (see details here)
  • [ X] documentation is changed or added

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2021

Codecov Report

Merging #451 (80be955) into master (38c0d9b) will increase coverage by 1.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
+ Coverage   90.48%   91.83%   +1.34%     
==========================================
  Files          12       12              
  Lines         778      796      +18     
==========================================
+ Hits          704      731      +27     
+ Misses         74       65       -9     
Impacted Files Coverage Δ
src/helpers.js 100.00% <100.00%> (ø)
src/transform.js 86.26% <100.00%> (+6.03%) ⬆️

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 38c0d9b...80be955. Read the comment docs.

@swain
Copy link

swain commented Oct 20, 2021

@karellm any update here? I think several people could benefit from this change being merged.

Copy link
Member

@karellm karellm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks good. My only concern is that I would like more test at the parser level. I don't like the code coverage reduction. Could you add some while you address my other comment about the README?

README.md Outdated Show resolved Hide resolved
@lars-mortenson
Copy link
Contributor Author

Thanks for the PR, it looks good. My only concern is that I would like more test at the parser level. I don't like the code coverage reduction. Could you add some while you address my other comment about the README?

No problem, I should have an update shortly.

@lars-mortenson lars-mortenson force-pushed the reset-default-value-locale branch 4 times, most recently from 916cd55 to f836445 Compare November 1, 2021 20:56
@lars-mortenson lars-mortenson force-pushed the reset-default-value-locale branch from f836445 to 80be955 Compare November 8, 2021 14:23
@swain
Copy link

swain commented Nov 8, 2021

@karellm any update on this PR? Looks like the changes you requested have been addressed.

@lars-mortenson
Copy link
Contributor Author

Hey @karellm, any remaining issues with these changes?

@karellm karellm merged commit 62bb98f into i18next:master Nov 14, 2021
@karellm
Copy link
Member

karellm commented Nov 14, 2021

Sorry for the delay. Thanks for the great work, I'm deploying it as 5.2.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.

4 participants