Repository navigation
destructure.js: fix @transifexKey nesting #1764
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -683,10 +683,58 @@ const rekeySource = (structured, transifexPaths) => { | |
| }; | ||
|
|
||
| const rekeyTranslations = (source, translated, transifexPaths) => { | ||
| for (const { sourcePath, transifexPath } of transifexPaths) | ||
| setPath(sourcePath, getPath(transifexPath, translated), translated); | ||
| // It's possible for one transifexPath to be a subpath of another, | ||
| // representing an ancestor message object. We want to rekey those shorter | ||
| // subpaths only after first rekeying the longer/deeper paths. Here, we | ||
| // reorder transifexPaths along those lines. Other than that though, we try to | ||
| // preserve the original order. | ||
| const ordered = [...transifexPaths]; | ||
| for (let i = 0; i < ordered.length - 1;) { | ||
| let changed = false; | ||
| for (let j = i + 1; j < ordered.length; j += 1) { | ||
| if (startsWith(ordered[i].transifexPath, ordered[j].transifexPath) && | ||
| !equals(ordered[i].transifexPath, ordered[j].transifexPath)) { | ||
| const ancestor = ordered[i]; // Subpath | ||
| const descendant = ordered[j]; // Longer/deeper path | ||
|
Comment on lines
+697
to
+698
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like I'm mixing metaphors here, referencing both subpaths and ancestors. |
||
| ordered.splice(j, 1); | ||
| ordered.splice(i, 1, descendant, ancestor); | ||
| changed = true; | ||
| break; | ||
| } | ||
| } | ||
| if (changed) | ||
| i = 0; // Start over | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that we really need to start over here, so I may remove this line. I wrote this code pretty fast shortly before release. This is definitely not the most efficient sort algorithm, but I'm also pretty sure that |
||
| else | ||
| i += 1; | ||
| } | ||
|
|
||
| // A single `transifexPath` can be copied to multiple `sourcePath`s. Here, we | ||
| // count how many times each `transifexPath` appears. | ||
| const pathCounts = new Map(); | ||
| for (const { transifexPath } of transifexPaths) { | ||
| if (!hasPath(transifexPath, source)) deletePath(transifexPath, translated); | ||
| const key = transifexPath.join('.'); | ||
| pathCounts.set(key, (pathCounts.get(key) ?? 0) + 1); | ||
| } | ||
| const decrementCount = (path) => { | ||
| const key = path.join('.'); | ||
| const newCount = pathCounts.get(key) - 1; | ||
| pathCounts.set(key, newCount); | ||
| return newCount; | ||
| }; | ||
|
|
||
| for (const { sourcePath, transifexPath } of ordered) { | ||
| if (hasPath(sourcePath, translated)) | ||
| throw new Error(`@transifexKey: attempted to copy the value at ${transifexPath.join('.')} to ${sourcePath.join('.')}, but there is already a value at ${sourcePath.join('.')}.`); | ||
| if (!hasPath(transifexPath, translated)) | ||
| throw new Error(`@transifexKey: attempted to copy the value at ${transifexPath.join('.')} to ${sourcePath.join('.')}, but there is no value at ${transifexPath.join('.')}.`); | ||
|
Comment on lines
+726
to
+729
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These errors aren't thrown right now, but they helped me track down the problem. |
||
|
|
||
| setPath(sourcePath, getPath(transifexPath, translated), translated); | ||
|
|
||
| // Delete the old transifexPath as soon as possible (as soon as the count is | ||
| // zero) in order to account for subpaths. We don't want to move a | ||
| // descendant message along with its ancestor message object. | ||
| if (!hasPath(transifexPath, source) && decrementCount(transifexPath) === 0) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: It was preexisting code, but maybe let's add a code comment explaining |
||
| deletePath(transifexPath, translated); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Explain why.