fix: add check for keepRemoved in failOnUpdate #907
Conversation
|
@cravend Thanks a lot for the PR. The fix looks good but can you please add a test to document the change? |
Codecov ReportPatch has no changes to coverable lines.
📢 Thoughts on this report? Let us know!. |
|
@karellm sure, but I don't see anywhere that any of the transform is tested. Where should I add the test? |
|
@cravend The transform is tested here: https://github.com/i18next/i18next-parser/blob/master/test/parser.test.js |
|
Just encountered this issue myself! Thanks @cravend. Shout if you want a hand finishing this PR |
src/transform.js
Outdated
| const unreferencedCount = this.options.keepRemoved ? oldCount : 0; | ||
| if (addCount + restoreCount + oldCount - unreferencedCount !== 0) { |
There was a problem hiding this comment.
This doesn't account for when keepRemoved is not a boolean. We use a regex to match 'common' namespace translations, so that parsing any package in our repo doesn't remove old common: translations.
keepRemoved: [/^common:/],
There was a problem hiding this comment.
@RobHannay Feel free to open a new PR with proper testing.
|
Hi all, sorry for my delayed response. I've just reworked the logic to handle when However, I had trouble writing tests for it, because it exits with code 1 and mocha interprets that as a failure and exits itself. I don't know enough about this testing framework to know how to ignore the exit code and continue the tests, but I wanted to push up a WIP to see if @RobHannay or @karellm either of you knew how to fix this. The tests aren't great (I created all test cases but didn't elaborate on them once I realized the issue with process.exit()). |
|
@cravend You can get some inspiration here: https://github.com/i18next/i18next-parser/pull/932/files |
|
Closing in favour of #948 |
Why am I submitting this PR
This issue adds a check for keepRemoved in the calculation of the files changed for failOnUpdate.
Does it fix an existing ticket?
Yes #666
Checklist
yarn test(see details here)