transifex/restructure: add context to whitespace errors#832
Open
alxndrsn wants to merge 10 commits intogetodk:masterfrom
Open
transifex/restructure: add context to whitespace errors#832alxndrsn wants to merge 10 commits intogetodk:masterfrom
alxndrsn wants to merge 10 commits intogetodk:masterfrom
Conversation
alxndrsn
changed the title
Member
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
Definitely a clearer description of the error!
bin/util/transifex.js
Outdated
Comment on lines
+87
to
+89
| console.error(` [${message}]`); // eslint-disable-line no-console | ||
| console.error(` [${''.padStart(badLength, '^').padStart(badWhitespace.index + badLength, ' ').padEnd(message.length, ' ')}]`); // eslint-disable-line no-console | ||
| throw new Error(`unexpected whitespace in message '${path}' at index ${badWhitespace.index} ("${message}")`); |
Member
There was a problem hiding this comment.
I feel like this doesn't fully account for the difference between message and form. As an example, for the pluralized message App User | App Users, the second plural form is the one with multiple spaces. Here, the entire message would be shown (both plural forms), but then the index from the match on only the second form would be used to position the ^^^ and describe the index of the unexpected whitespace.
How about something like:
const error = forms.length === 1
? `unexpected white space in message '${path}'`
: `unexpected white space in plural form of message '${path}'`;
console.error(`${error}:`); // eslint-disable-line no-console
const badLength = badWhitespace[0].length;
console.error(` [${form}]`); // eslint-disable-line no-console
console.error(` [${''.padStart(badLength, '^').padStart(badWhitespace.index + badLength, ' ').padEnd(message.length, ' ')}]`); // eslint-disable-line no-console
if (forms.length !== 1)
console.error(`The entire message is: [${message}]`); // eslint-disable-line no-console
throw new Error(`${error} at index ${badWhitespace.index} ("${form}")`);
Contributor
Author
There was a problem hiding this comment.
Good catch.
I've updated to consider the index of the form. Some example errors:
single-value
unexpected white space in translation string 'datasets':
[Data sets]
[ ^^ ]
...
Error: unexpected whitespace in message 'datasets' at index 4 ("Data sets")
singular of multi-value
unexpected white space in translation string 'appUser[0]':
[App User]
[ ^^ ]
...
Error: unexpected whitespace in message 'appUser[0]' at index 3 ("App User | App Users")
plural of multi-value
unexpected white space in translation string 'appUser[1]':
[App Users]
[ ^^ ]
...
Error: unexpected whitespace in message 'appUser[1]' at index 3 ("App User | App Users")
ec25972 to
f47d54a
Compare
added 2 commits
September 6, 2023 09:21
Member
|
I'll take a look at this one during v2025.3! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Example
Add a new translation string:
Current
masterThis PR