i18next / i18next-parser

Parse your code to extract translation keys/values and manage your catalog files
MIT License
473 stars 195 forks source link

fix: add check for keepRemoved in failOnUpdate #907

Closed cravend closed 8 months ago

cravend commented 1 year ago

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

karellm commented 1 year ago

@cravend Thanks a lot for the PR. The fix looks good but can you please add a test to document the change?

codecov-commenter commented 1 year ago

Codecov Report

Patch has no changes to coverable lines.

Files Changed Coverage
src/transform.js 0.00%

:loudspeaker: Thoughts on this report? Let us know!.

cravend commented 1 year ago

@karellm sure, but I don't see anywhere that any of the transform is tested. Where should I add the test?

karellm commented 1 year ago

@cravend The transform is tested here: https://github.com/i18next/i18next-parser/blob/master/test/parser.test.js

RobHannay commented 12 months ago

Just encountered this issue myself! Thanks @cravend. Shout if you want a hand finishing this PR

cravend commented 10 months ago

Hi all, sorry for my delayed response. I've just reworked the logic to handle when keepRemoved is a regex. I tested locally (yarn link, running it inside/outside of a regex match, as well as with/without a "changed" translation), and the behavior seemed correct to me.

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()).

karellm commented 10 months ago

@cravend You can get some inspiration here: https://github.com/i18next/i18next-parser/pull/932/files

karellm commented 8 months ago

Closing in favour of #948