-
Notifications
You must be signed in to change notification settings - Fork 184
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
Make more strings translatable #685
Conversation
js/output/sql/sql-output.js
Outdated
@@ -65,95 +65,97 @@ window.SQLOutput = Backbone.View.extend({ | |||
statement = statement || ""; | |||
statement = statement.toUpperCase(); | |||
|
|||
// Error messages take form: 'near \"%T\": syntax error' |
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.
I grepped the SQLite C source, it looks like they all take this form unless it's in fulltextsearch mode, and sqlite.js doesn't seem to enable that by default (https://github.com/kripken/sql.js/pull/199)
js/output/sql/sql-output.js
Outdated
} | ||
|
||
// Possible SELECT with missing FROM | ||
if (errorMessage.indexOf("no such column:") !== -1 && | ||
statement.indexOf("SELECT") !== -1 && | ||
statement.indexOf("FROM") === -1) { | ||
errorMessage += ". " + i18n._("Are you missing a FROM clause?"); | ||
errorMessage += " " + i18n._("Are you missing a FROM clause?"); |
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.
I moved the period into the original sentence, seems better?
results: results | ||
results: results, | ||
databaseMsg: i18n._("Database Schema"), | ||
resultsMsg: i18n._("Query results"), |
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.
I changed from "Results" to "Query results" for more uniqueness in Crowdin
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.
Oh, this is great, thanks for thinking about this. <3
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.
I've got one suggestion for handling the SQL error message, and have one concern about how the existing code searches the already-translated error message to add more details about the error.
But neither one is a blocker for me.
js/output/sql/sql-output.js
Outdated
var isSyntaxError = errorMessage.indexOf(": syntax error") > -1; | ||
if (isSyntaxError) { | ||
errorMessage = i18n._("There's a syntax error " + | ||
errorMessage.split(":")[0]); | ||
const nearPhrase = errorMessage.split(":")[0]; |
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.
If you're concerned about future-proofing this, you could use a regexp to match the expected form of the error message and extract the phrase, and then if it doesn't match just pass the entire message.
js/output/sql/sql-output.js
Outdated
errorMessage.split(":")[0]); | ||
const nearPhrase = errorMessage.split(":")[0]; | ||
errorMessage = i18n._("There's a syntax error near %(nearThing)s.", | ||
{nearThing: nearPhrase.substr(5)}); | ||
} | ||
|
||
// Possible SELECT with missing FROM | ||
if (errorMessage.indexOf("no such column:") !== -1 && |
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.
It seems unlikely to cause a problem, but this line is now searching through a translated string for the error message. Would it be better here (and a few places below as well) to use your new nearPhrase value? (Though I don't understand how "no such column" is works with "near", so maybe that is in some other part of the error message? It almost looks, actually, like this "no such column" thing will never match.
Thanks so much to both! I've just had a look into computer.pot and some of these strings are already there (I guess that is expected). However, there are also a couple of strings that seem like they should not be available for translations. They are all visible here: https://crowdin.com/translate/khanacademy/26342/enus-cs#q=debugger.handlebars For example:
I am not sure how much this is related to this PR but just FYI. |
Oh actually, in string Step over, Caroline Charrow wrote:
It seems that at least "Debug Mode" could be hidden in this way as well. The other words/phrases are perhaps too generic and could potentially appear elsewhere in the future... (Sorry for out-of-place comments :-/ ) |
@danielhollas I've gone and hidden the other strings from debugger.handlebars. I also have a separate PR that removes that from the code path. @davidflanagan Thanks for your helpful comments, I'll address them shortly. |
@davidflanagan I've made a few changes:
To check my changes, I went through the SQL tests and added the full error message to every failingTest, where they previously only checked the helper messages. I found a number of messages that needed to be i18n'ed that way. Regex matching would be more future-proof, but given the many tests, it feels relatively safe to use the indexOf/split. There are likely still some error messages that aren't translated. It isn't particularly easy to go through the C code to find all the messages/situations to cover. This should cover a huge % though. |
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.
I like the new organization of the error message parsing a lot. I don't think you need another approval, but here it is since I took a look.
i18n._("Do you have a semi-colon after each statement?"); | ||
} else if (isSyntaxError && | ||
statement.indexOf("INSERT") !== -1 && | ||
statement.search(/[^INSERT],\d*\s*[a-zA-Z]+/) > -1 | ||
statement.search(/[^INSERT],\d*\s*[a-zA-Z]+/) > -1 |
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.
Not related to this patch, but I can't imagine that [^INSERT]
is actually what is wanted in this regexp
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.
Hm, it matches the desired text cases, when INSERT is at the start of the line (even if its the second line). I don't want to mess with it in case there was a reason I used the ^ .
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.
But the square brackets change the caret from a start of line anchor to a NOT operator and the result is that you are matching any one character that is not one of I, N, S, E, R, or T followed by a comma, optional digits, optional spaces and letters. So "blah INSERT Q,a" matches but "blah INSERT R,a" does not match
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.
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.
:-)
js/output/sql/sql-output.js
Outdated
} | ||
const uniqError = sqliteError.indexOf("UNIQUE constraint failed:") > -1; | ||
if (uniqError) { | ||
const colName = sqliteError.split(":")[1].trim(); |
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.
One more thought about split... In most or all of these cases you could split on the string that you just matched with indexOf, and then you'd be sure you were getting the right part of the string even if sqllite adds some kind of "ERROR:" prefix to the string. That seems unlikely and you have a lot of tests, so I don't feel like you need to make this change, and am just mentioning it because the idea of splitting on a relatively long string is not something I've done before and I thought it was interesting :-)
@davidflanagan Changed the split to split on the same string that indexOf is checking. |
Huh, OK, I need to spend more quality time with regexpal.com. Thanks for
the observation! I'll file a bug.
…On Fri, Sep 21, 2018 at 11:10 AM David Flanagan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In js/output/sql/sql-output.js
<#685 (comment)>:
> i18n._("Do you have a semi-colon after each statement?");
} else if (isSyntaxError &&
statement.indexOf("INSERT") !== -1 &&
- statement.search(/[^INSERT],\d*\s*[a-zA-Z]+/) > -1
+ statement.search(/[^INSERT],\d*\s*[a-zA-Z]+/) > -1
:-)
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#685 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASIUnqmaHO0IjDCroudhyssTuFjgz0kks5udSt7gaJpZM4WwACl>
.
|
:D I had no idea that xkcd is a valid review comment :D Thanks again to both for your work, much appreciated! Can I ask how many new strings can we expect to appear in Crowdin? I am just wondering whether this deserves an announcement on the forum to notify others. |
I posted in the forum, thanks for the note about that, Daniel. This change
is now deployed, so I imagine they show up in Crowdin sometime soon.
…On Fri, Sep 21, 2018 at 5:16 PM Daniel Hollas ***@***.***> wrote:
:D I had no idea that xkcd is a valid review comment :D
Thanks again to both for your work, much appreciated!
Can I ask how many new strings can we expect to appear in Crowdin? I am
just wondering whether this deserves an announcement on the forum to notify
others.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#685 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASIUkr9dXjqH-BHhekDFW6xv1oWMJ4Oks5udYFVgaJpZM4WwACl>
.
|
@pamelafox I think there's one more string that is currently untranslatable in SQL output. "row" or "rows" in the database header. I think it sits in here: live-editor/tmpl/sql-results.handlebars Line 95 in a4edcb1
Let me know if I should open a new issue here or a JIRA ticket. Also, many thanks for taking part in today's advocate sessions and for your generous offer! ❤️ |
High-level description of change
The strings in Handlebars templates aren't translatable by webapp, so this moves the rest of them out. I did not move them out of live-editor.handlebars, as webapp does not use that file, it overrides the rendering with a React output.
I also fixed an issue with an untranslatable SQL error message.
Are there performance implications for this change?
No
Have you added tests to cover this new/updated code?
No tests added. I ran existing tests, which includes a test for SQL error messages. No tests for the UI, so I manually tested those.
Risks involved
I'm most nervous about the SQL change, as it assumes the format of the string. However, I checked the C code that it's compiled from, and the format seems consistent.
Are there any dependencies or blockers for merging this?
No
How can we verify that this change works?
Note that the modal doesn't work in this repo, it only works in webapp. Apparently we never included bootstrap-modal here.