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

fix(extract): when files are used, don't overwrite obsolete #1964

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nicolas-cherel
Copy link

@nicolas-cherel nicolas-cherel commented Jun 21, 2024

Description

When using lingui extract <...files> all translations in catalog that are not in the listed file get their obsolete flag to false, regardless if the obsolete flag was not set or already set to true.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

Fixes # (issue)
#1963

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation (if appropriate)

Copy link

vercel bot commented Jun 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 25, 2024 2:39pm

Copy link

github-actions bot commented Jun 21, 2024

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.88 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.68 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

@andrii-bodnar andrii-bodnar linked an issue Jun 21, 2024 that may be closed by this pull request
@nicolas-cherel nicolas-cherel changed the title Extract files obsolete fix fix(extract): when files are used, don't overwrite obsolote Jun 21, 2024
@nicolas-cherel nicolas-cherel changed the title fix(extract): when files are used, don't overwrite obsolote fix(extract): when files are used, don't overwrite obsolete Jun 21, 2024
@andrii-bodnar
Copy link
Contributor

@nicolas-cherel thanks for the contribution! Please include tests that cover this use case

@timofei-iatsenko
Copy link
Collaborator

i would recommend to add full e2e test here packages/cli/test

  1. Create a folder with test case name
  2. In fixtures put test files to extract (you can copy from sibling test case)
  3. in existing put catalogs with already extracted messages an obsolete flag
  4. in expected put how catalogs should like after extracting
  5. add separate describe into packages/cli/test/index.test.ts and run extractCommand with parameters you need

@nicolas-cherel
Copy link
Author

Note: it seem that only the lingui json format has support for the obsolete flag, I hope depending on it for the tests is not a issue

@timofei-iatsenko
Copy link
Collaborator

Po files also support obsolete, i would prefer to have it with PO files, lingui json format is not recommended to use by docs.

@nicolas-cherel
Copy link
Author

nicolas-cherel commented Jun 24, 2024

Ok I must have missed something on my first pass, I'll get right away now that I have the json version set up

@nicolas-cherel
Copy link
Author

nicolas-cherel commented Jun 24, 2024

Ok, note, we previously had an issue specific to the json files where a "obsolete": true was added to translations with partial extract, which does not happen with .po files due to the format specificities. I manage to witness the obsolete flag being removed with the partial extract though, so that fix is confirmed.

I renamed the test suite fixtures/exisiting/expected folder to extract-partial-consitency as the general problem was more a consistency issue with whole and partial extracts generating inconsistent outputs.

@nicolas-cherel
Copy link
Author

re-pushed for a typo fix on extract-partial-consistency folder

Comment on lines +33 to +37
if (glob.sync(existingPath).length === 1) {
await fs.cp(existingPath, actualPath, { recursive: true })
}

return { rootDir, actualPath, existingPath, expectedPath }
Copy link
Collaborator

Choose a reason for hiding this comment

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

since now its implemented for all test cases, remove

      await fs.cp(
        nodepath.join(rootDir, "existing"),
        nodepath.join(rootDir, "actual"),
        { recursive: true }
      )

from this spec:

it("should extract and clean obsolete", 

@andrii-bodnar andrii-bodnar marked this pull request as draft August 1, 2024 06:35
@andrii-bodnar
Copy link
Contributor

@nicolas-cherel could you please address the discussion above?

@nicolas-cherel
Copy link
Author

sorry, I was not available on july, and forgot to get back to the issue, I'll try to do it ASAP

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.

lingui extract <..files> overwrites obsolete flag
3 participants