destructure.js: fix @transifexKey nesting - #1764
matthew-white wants to merge 1 commit into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
| } | ||
| } | ||
| if (changed) | ||
| i = 0; // Start over |
There was a problem hiding this comment.
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 .sort() can't handle this case. Given that, and given the small size of transifexPaths, I think code clarity is more important than sort efficiency.
| const ancestor = ordered[i]; // Subpath | ||
| const descendant = ordered[j]; // Longer/deeper path |
There was a problem hiding this comment.
I feel like I'm mixing metaphors here, referencing both subpaths and ancestors.
| 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('.')}.`); |
There was a problem hiding this comment.
These errors aren't thrown right now, but they helped me track down the problem.
|
@alxndrsn, you may be interested in this PR, since you've been helping to get destructure.js back to a runnable state. I was thinking to tag Sadiq for code review, since he's familiar with the |
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
Adding TODO comments
| // representing an ancestor message object. We want to rekey those shorter | ||
| // subpaths only after first rekeying the longer/deeper paths. Here, we |
There was a problem hiding this comment.
We want to rekey those shorter subpaths only after first rekeying the longer/deeper paths.
TODO: Explain why.
| // 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) |
There was a problem hiding this comment.
TODO: It was preexisting code, but maybe let's add a code comment explaining !hasPath(transifexPath, source). I think it's the difference between copying vs. moving a message. If a message is just copied, it will still exist at its original path in source.
OK, here we go! The issue arises for The central-frontend/apps/central/src/components/form/upload.vue Lines 297 to 346 in 27b01d9 The problem is that those translations are a large message object, and we already had individual messages that were separately moved to central-frontend/apps/central/src/locales/en.json5 Lines 234 to 235 in 27b01d9 central-frontend/apps/central/src/components/form/new-page.vue Lines 49 to 51 in 27b01d9 What I think the problem is (what happens without this PR):
What this PR does instead:
|
I hadn't realised it was fundamentally broken! I think it's definitely time this code got some tests. |
|
Yeah, this case introduced with #1535 isn't something that destructure.js is able to handle. 😢 We could easily resolve it just by removing |
Related: getodk/central#2151. Once we get the required testing infrastructure in place, we can add one or more tests about the specific case that this PR fixes. |
This branch has not been deployed
2 participants
Closes getodk/central#2127.
There's some description of the problem in getodk/central#2127 and in code comments, but I plan to add more detail to this PR. It's kind of a complex case.
What has been done to verify that this works as intended?
After this change (along with #1751), destructure.js at last runs without error. 🎉 I used this code in the previous release to run destructure.js, and the resulting Vue I18n JSON matched my expectations.
Given the complexity of the case, I'm tempted to add the first tests of our Transifex scripts. Doing so would probably help document this case.
Why is this the best possible solution? Were any other approaches considered?
I'll add more detail later explaining the underlying problem, after which I think the code approach will make more sense. The code itself isn't optimized, which is one reason why I've marked this PR as draft.
getodk/central#2127 proposes two main approaches: handling this case vs. detecting and disallowing it. This PR implements the former approach (handling this case), but the latter approach would probably be more straightforward. I'll probably continue to run with the former approach for now, since I've already written code for it. However, I may turn to the latter approach (just detecting this case) if I feel like it'd require fewer new tests.