Enforce message element indentation #78
Conversation
stasm
left a comment
stasm
left a comment
There was a problem hiding this comment.
Thanks, r+. Can you respond to my questions below before merging?
| this._index = lineStart; | ||
|
|
||
| if (ch === '#') { | ||
| if (attrs !== null) { |
There was a problem hiding this comment.
Do you want to remove this check and the error?
There was a problem hiding this comment.
Yeah. Since we don't support both, let's just consume one or the other and move on. This simplifies the code and we only pay with a different error message that gives less of a clue, which I think is ok, since our runtime errors are not meaningful at all anyway (since we optimize for perf).
There was a problem hiding this comment.
I agree we shouldn't care about the error itself, but the important thing here is to make the runtime parser yield the same Junks as the full syntax parser. Otherwise, as a developer you might write something which will work in testing, but will break in Pontoon / compare-locales etc.
There was a problem hiding this comment.
@zbraniecki made me realize that the if-else in lines 162-166 is enough to junkify an erroneous message here.
| this.skipInlineWS(); | ||
| val = this.getPattern(); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Is this else block an optimization for the most common use-case of a one-line key = value? If so, can you add a comment explaining it, please?
2 participants
Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1405645
@stasm - one trick is that I don't know how to test such cases in our runtime library. Seems like I need to test for errors and AST, but the tests don't seem to handle that right now.
So, maybe it's worth it to just land it since it passes all tests and we manually tested that it works, and once we improve the infra, add tests for that?