[localize-tools] Improve handling of duplicate msg id #2405

augustjk · 2022-01-14T19:07:24Z

Fixes #2140 and #1897.

For messages to be considered same:
- No longer check for expressions to be identical.
- Do check that desc be identical.
Display a more helpful aggregate error message for all messages with the same id.
For XLB format, add placeholder index to name attribute of <ph> tags.
Utilize the placeholder indexes embedded in tags for determining placeholder locations during transform mode.

changeset-bot · 2022-01-14T19:07:26Z

🦋 Changeset detected

Latest commit: 4df4ff1

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

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

github-actions · 2022-01-14T19:07:47Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -1% - +2% (-0.16ms - +0.57ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -1% - +1% (-0.52ms - +0.93ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -1% - +1% (-0.18ms - +0.21ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +1% (-0.33ms - +0.10ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-1.00ms - +1.33ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +1% (-0.83ms - +0.63ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -1% - +1% (-4.46ms - +4.72ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -4% - +1% (-3.39ms - +1.03ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -2% - +0% (-6.12ms - +1.38ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +1% (-1.09ms - +0.73ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +1% (-1.94ms - +3.64ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -1% - +1% (-6.69ms - +5.79ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +1% (-2.11ms - +4.60ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 97.0.4692.71
Sample size: 50
Built by: Benchmarks #2259
Commit: 819db45

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	73.81ms - 74.96ms	-	unsure 🔍 -1% - +1% -0.52ms - +0.93ms	faster ✔ 20% - 22% 18.88ms - 20.60ms
tip-of-tree tip-of-tree	73.74ms - 74.63ms	unsure 🔍 -1% - +1% -0.93ms - +0.52ms	-	faster ✔ 20% - 22% 19.17ms - 20.72ms
previous-release previous-release	93.49ms - 94.77ms	slower ❌ 25% - 28% 18.88ms - 20.60ms	slower ❌ 26% - 28% 19.17ms - 20.72ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	649.71ms - 656.53ms	-	unsure 🔍 -1% - +1% -4.46ms - +4.72ms	faster ✔ 7% - 8% 46.84ms - 57.06ms
tip-of-tree tip-of-tree	649.91ms - 656.06ms	unsure 🔍 -1% - +1% -4.72ms - +4.46ms	-	faster ✔ 7% - 8% 47.19ms - 56.98ms
previous-release previous-release	701.26ms - 708.88ms	slower ❌ 7% - 9% 46.84ms - 57.06ms	slower ❌ 7% - 9% 47.19ms - 56.98ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	721.85ms - 730.89ms	-	unsure 🔍 -1% - +1% -6.69ms - +5.79ms	faster ✔ 3% - 4% 19.82ms - 33.17ms
tip-of-tree tip-of-tree	722.52ms - 731.13ms	unsure 🔍 -1% - +1% -5.79ms - +6.69ms	-	faster ✔ 3% - 4% 19.51ms - 32.58ms
previous-release previous-release	747.95ms - 757.78ms	slower ❌ 3% - 5% 19.82ms - 33.17ms	slower ❌ 3% - 4% 19.51ms - 32.58ms	-

lit-html-kitchen-sink

Browser: chrome-headless 97.0.4692.71
Sample size: 30
Built by: Benchmarks #2259
Commit: 819db45

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	28.68ms - 28.96ms	-	unsure 🔍 -1% - +1% -0.18ms - +0.21ms	faster ✔ 15% - 17% 4.91ms - 5.86ms
tip-of-tree tip-of-tree	28.67ms - 28.94ms	unsure 🔍 -1% - +1% -0.21ms - +0.18ms	-	faster ✔ 15% - 17% 4.93ms - 5.87ms
previous-release previous-release	33.75ms - 34.66ms	slower ❌ 17% - 20% 4.91ms - 5.86ms	slower ❌ 17% - 20% 4.93ms - 5.87ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	77.83ms - 80.22ms	-	unsure 🔍 -4% - +1% -3.39ms - +1.03ms	unsure 🔍 -6% - +1% -4.55ms - +0.89ms
tip-of-tree tip-of-tree	78.34ms - 82.07ms	unsure 🔍 -1% - +4% -1.03ms - +3.39ms	-	unsure 🔍 -5% - +3% -3.73ms - +2.43ms
previous-release previous-release	78.41ms - 83.30ms	unsure 🔍 -1% - +6% -0.89ms - +4.55ms	unsure 🔍 -3% - +5% -2.43ms - +3.73ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	25.11ms - 25.72ms	-	unsure 🔍 -1% - +2% -0.16ms - +0.57ms	faster ✔ 10% - 12% 2.82ms - 3.59ms
tip-of-tree tip-of-tree	25.01ms - 25.40ms	unsure 🔍 -2% - +1% -0.57ms - +0.16ms	-	faster ✔ 11% - 13% 3.11ms - 3.72ms
previous-release previous-release	28.38ms - 28.86ms	slower ❌ 11% - 14% 2.82ms - 3.59ms	slower ❌ 12% - 15% 3.11ms - 3.72ms	-

lit-html-repeat

Browser: chrome-headless 97.0.4692.71
Sample size: 30
Built by: Benchmarks #2259
Commit: 819db45

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.16ms - 11.45ms	-	unsure 🔍 -3% - +1% -0.33ms - +0.10ms	faster ✔ 10% - 13% 1.32ms - 1.62ms
tip-of-tree tip-of-tree	11.26ms - 11.58ms	unsure 🔍 -1% - +3% -0.10ms - +0.33ms	-	faster ✔ 9% - 12% 1.19ms - 1.52ms
previous-release previous-release	12.73ms - 12.82ms	slower ❌ 12% - 15% 1.32ms - 1.62ms	slower ❌ 10% - 13% 1.19ms - 1.52ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	279.55ms - 283.24ms	-	unsure 🔍 -2% - +0% -6.12ms - +1.38ms	faster ✔ 29% - 31% 117.66ms - 124.25ms
tip-of-tree tip-of-tree	280.50ms - 287.03ms	unsure 🔍 -0% - +2% -1.38ms - +6.12ms	-	faster ✔ 29% - 30% 114.33ms - 122.84ms
previous-release previous-release	399.62ms - 405.08ms	slower ❌ 42% - 44% 117.66ms - 124.25ms	slower ❌ 40% - 44% 114.33ms - 122.84ms	-

lit-html-template-heavy

Browser: chrome-headless 97.0.4692.71
Sample size: 50
Built by: Benchmarks #2259
Commit: 819db45

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	52.45ms - 54.63ms	-	unsure 🔍 -2% - +2% -1.00ms - +1.33ms	faster ✔ 14% - 17% 8.57ms - 11.13ms
tip-of-tree tip-of-tree	52.97ms - 53.79ms	unsure 🔍 -2% - +2% -1.33ms - +1.00ms	-	faster ✔ 15% - 17% 9.23ms - 10.80ms
previous-release previous-release	62.72ms - 64.06ms	slower ❌ 16% - 21% 8.57ms - 11.13ms	slower ❌ 17% - 20% 9.23ms - 10.80ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	113.98ms - 115.20ms	-	unsure 🔍 -1% - +1% -1.09ms - +0.73ms	faster ✔ 11% - 13% 14.72ms - 17.26ms
tip-of-tree tip-of-tree	114.10ms - 115.44ms	unsure 🔍 -1% - +1% -0.73ms - +1.09ms	-	faster ✔ 11% - 13% 14.50ms - 17.11ms
previous-release previous-release	129.46ms - 131.69ms	slower ❌ 13% - 15% 14.72ms - 17.26ms	slower ❌ 13% - 15% 14.50ms - 17.11ms	-

reactive-element-list

Browser: chrome-headless 97.0.4692.71
Sample size: 50
Built by: Benchmarks #2259
Commit: 819db45

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.96ms - 52.98ms	-	unsure 🔍 -2% - +1% -0.83ms - +0.63ms	unsure 🔍 -1% - +1% -0.70ms - +0.57ms
tip-of-tree tip-of-tree	52.04ms - 53.09ms	unsure 🔍 -1% - +2% -0.63ms - +0.83ms	-	unsure 🔍 -1% - +1% -0.61ms - +0.69ms
previous-release previous-release	52.15ms - 52.90ms	unsure 🔍 -1% - +1% -0.57ms - +0.70ms	unsure 🔍 -1% - +1% -0.69ms - +0.61ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	654.84ms - 659.08ms	-	unsure 🔍 -0% - +1% -1.94ms - +3.64ms	unsure 🔍 -0% - +0% -2.83ms - +2.71ms
tip-of-tree tip-of-tree	654.29ms - 657.92ms	unsure 🔍 -1% - +0% -3.64ms - +1.94ms	-	unsure 🔍 -1% - +0% -3.46ms - +1.63ms
previous-release previous-release	655.23ms - 658.80ms	unsure 🔍 -0% - +0% -2.71ms - +2.83ms	unsure 🔍 -0% - +1% -1.63ms - +3.46ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	763.20ms - 768.19ms	-	unsure 🔍 -0% - +1% -2.11ms - +4.60ms	unsure 🔍 -0% - +1% -2.41ms - +4.38ms
tip-of-tree tip-of-tree	762.21ms - 766.70ms	unsure 🔍 -1% - +0% -4.60ms - +2.11ms	-	unsure 🔍 -0% - +0% -3.47ms - +2.96ms
previous-release previous-release	762.41ms - 767.02ms	unsure 🔍 -1% - +0% -4.38ms - +2.41ms	unsure 🔍 -0% - +0% -2.96ms - +3.47ms	-

_{tachometer-reporter-action v2 for Benchmarks}

aomarks

Great! A few small comments but otherwise LGTM.

.changeset/many-singers-complain.md

packages/localize-tools/src/tests/analysis.unit.test.ts

packages/localize-tools/src/program-analysis.ts

packages/localize-tools/src/tests/analysis.unit.test.ts

packages/localize-tools/src/messages.ts

packages/localize-tools/src/modes/transform.ts

packages/localize-tools/src/tests/transform.unit.test.ts

aomarks

Looks great! I think we're good to go with #2140, but I believe #1897 will need to be fixed as part of this PR too to avoid breaking the XLB use case.

packages/localize-tools/src/modes/transform.ts

aomarks · 2022-01-24T17:26:30Z

packages/localize-tools/src/messages.ts

@@ -66,6 +66,7 @@ export interface Bundle {
 */
 export interface Placeholder {
  untranslatable: string;
+  index?: number;


This should be required (related to other comment).

packages/localize-tools/src/formatters/xlb.ts

- For messages to be considered same: - No longer check for expressions to be identical - Do check that desc be identical - Displays a more helpful aggregate error message for all messages with the same id

aomarks

Awesome! Looks great. Just one suggestion to mention the XLB breaking change in the changelog too.

.changeset/many-singers-complain.md

augustjk requested a review from aomarks January 14, 2022 19:07

augustjk requested a review from justinfagnani as a code owner January 14, 2022 19:07

aomarks requested changes Jan 14, 2022

View reviewed changes

.changeset/many-singers-complain.md Outdated Show resolved Hide resolved

packages/localize-tools/src/tests/analysis.unit.test.ts Show resolved Hide resolved

packages/localize-tools/src/program-analysis.ts Outdated Show resolved Hide resolved

augustjk requested a review from aomarks January 14, 2022 19:57

aomarks reviewed Jan 14, 2022

View reviewed changes

packages/localize-tools/src/tests/analysis.unit.test.ts Outdated Show resolved Hide resolved

augustjk requested a review from aomarks January 20, 2022 23:25

aomarks requested changes Jan 20, 2022

View reviewed changes

augustjk requested a review from aomarks January 21, 2022 19:34

aomarks requested changes Jan 24, 2022

View reviewed changes

augustjk commented Jan 24, 2022

View reviewed changes

packages/localize-tools/src/formatters/xlb.ts Outdated Show resolved Hide resolved

augustjk and others added 10 commits January 24, 2022 15:08

[localize-tools] Improve handling of duplicate msg id

7371443

- For messages to be considered same: - No longer check for expressions to be identical - Do check that desc be identical - Displays a more helpful aggregate error message for all messages with the same id

Add changeset

2c29d83

Address PR comments

42c763a

Handle transform of duplicate messages with different expressions

06eaacb

Use typescript printer for comparing expressions

8237e2b

Make type of placeholder index a number

17b32d0

Expression matching that doesn't depend on source code

f5c8d3f

Add ph index for xlb and make index required in Placeholder type

2b33c16

Throw on missing index for xlb

d9f2afb

Add changeset for xlb format changes

631decd

augustjk force-pushed the localize-tools-improve-dedupe branch from 6a65213 to 631decd Compare January 24, 2022 23:09

augustjk requested a review from aomarks January 24, 2022 23:22

Fix build-runtime-xlb test

4df4ff1

aomarks approved these changes Jan 25, 2022

View reviewed changes

.changeset/many-singers-complain.md Show resolved Hide resolved

augustjk linked an issue Jan 25, 2022 that may be closed by this pull request

[localize] Generate IDs for XLB placeholders #1897

Closed

augustjk merged commit 4a4afa7 into main Jan 25, 2022

augustjk deleted the localize-tools-improve-dedupe branch January 25, 2022 01:20

github-actions bot mentioned this pull request Jan 25, 2022

Version Packages #2448

Merged

augustjk mentioned this pull request Mar 31, 2022

[localize-tools] Keep expressions in attribute values for transform mode #2692

Merged

augustjk mentioned this pull request Jan 23, 2024

[localize-tools] Validation of existing translation should not fail on changed expression #4502

Closed

augustjk mentioned this pull request Jan 30, 2024

[localize] Make placeholder validation pass with different expressions #4530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[localize-tools] Improve handling of duplicate msg id #2405

[localize-tools] Improve handling of duplicate msg id #2405

augustjk commented Jan 14, 2022 •

edited

changeset-bot bot commented Jan 14, 2022 •

edited

github-actions bot commented Jan 14, 2022 •

edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

aomarks left a comment

aomarks left a comment

aomarks Jan 24, 2022

aomarks left a comment

[localize-tools] Improve handling of duplicate msg id #2405

[localize-tools] Improve handling of duplicate msg id #2405

Conversation

augustjk commented Jan 14, 2022 • edited

changeset-bot bot commented Jan 14, 2022 • edited

🦋 Changeset detected

github-actions bot commented Jan 14, 2022 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

aomarks left a comment

Choose a reason for hiding this comment

aomarks left a comment

Choose a reason for hiding this comment

aomarks Jan 24, 2022

Choose a reason for hiding this comment

aomarks left a comment

Choose a reason for hiding this comment

augustjk commented Jan 14, 2022 •

edited

changeset-bot bot commented Jan 14, 2022 •

edited

github-actions bot commented Jan 14, 2022 •

edited