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

Update existing XLIFF files #3514

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Update existing XLIFF files #3514

merged 3 commits into from
Dec 13, 2022

Conversation

enqueue
Copy link
Contributor

@enqueue enqueue commented Dec 9, 2022

lit-localize updates existing XLIFF translations files, if available, instead of re-generating these files from scratch.

Remarks about this change:

All note elements that result from a lit-localize run, will receive a from="lit-localize" attribute (see from. This is necessary to identify the note we are allowed to update in a subsequent run. Other tools might have added further note elements. However, this also means, that any existing XLIFF files will be changed once this version of lit-localize is run.

We could invest a lot more time into trying to preserve indentation. Other tools might reformat the whole file using tabs or five spaces or... Guessing the most probable level of indentation for new elements, based on surrounding elements, might be doable, but I hope that such an effort is not called for yet.

I adjusted the existing test case, but please feel free to suggest more scenarios we need to cover.

Fixes #1879
Fixes #3439

@changeset-bot
Copy link

changeset-bot bot commented Dec 9, 2022

🦋 Changeset detected

Latest commit: b535b56

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lit/localize-tools Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-cla
Copy link

google-cla bot commented Dec 9, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@enqueue enqueue marked this pull request as ready for review December 9, 2022 07:34
Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

This is fantastic, thank you for the PR! Code and test updates look great to me.

The only missing thing is a changeset file, which is how we manage releases.

If you run npx changeset add from the root of the monorepo, then a wizard should walk you through what to do:

  • You'll want to mark @lit/localize-tools as having a patch bump (would usually be minor, but we're still on major version 0).
  • I believe there are two changes, you could write something like this (or exactly this):
    - Existing XLIFF files will be updated instead of re-generated, so additional data added by other processes (such as `state="translated"` attributes) will be preserved instead of deleted.
    - XLIFF `<note>` elements generated by Lit Localize will now have a `from="lit-localize"` attribute to distinguish them from other notes.
    

Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Also looks like there are some test failures:

https://github.com/lit/lit/actions/runs/3655389797/jobs/6227289104

@enqueue
Copy link
Contributor Author

enqueue commented Dec 13, 2022

I added the changeset as suggested.

Adjusted the code to make the tests work. I was sure I had run them before committing the change (npm run test in the localize-tools packages), but perhaps I was looking at a cached result. Hope this will work fine now.

@enqueue enqueue requested review from aomarks and removed request for justinfagnani December 13, 2022 10:06
Copy link
Member

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@aomarks aomarks merged commit 7881171 into lit:main Dec 13, 2022
@lit-robot lit-robot mentioned this pull request Dec 15, 2022
43081j pushed a commit to 43081j/lit-html that referenced this pull request Jan 4, 2023
`lit-localize` updates existing XLIFF translations files, if available, instead of re-generating these files from scratch.

Remarks about this change:

All `note` elements that result from a `lit-localize` run, will receive a `from="lit-localize"` attribute (see [from](http://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#from). This is necessary to identify the note we are allowed to update in a subsequent run. Other tools might have added further `note` elements. However, this also means, that any existing XLIFF files will be changed once this version of `lit-localize` is run.

We could invest a lot more time into trying to preserve indentation. Other tools might reformat the whole file using tabs or five spaces or... Guessing the most probable level of indentation for new elements, based on surrounding elements, might be doable, but I hope that such an effort is not called for yet.

I adjusted the existing test case, but please feel free to suggest more scenarios we need to cover.

Fixes lit#1879
Fixes lit#3439
This was referenced Jan 9, 2023
@aomarks
Copy link
Member

aomarks commented Jan 9, 2023

This PR was just released in @lit/localize-tools@0.6.6: #3556 Thanks again!

aomarks added a commit that referenced this pull request Jan 17, 2023
Fixes a bug introduced in #3514 where we were not correctly updating XLIFF messages that contained placeholders, because of an index-invalidation bug in the way we were deleting the contents of existing nodes.

Fixes #3569
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants