Experimental support for #21 #173
Conversation
There was a problem hiding this comment.
Thanks! Generally I think this looks good. I suggested a few small changes that would improve the code.
After you address those, would you mind resubmitting this request without including the stuff that has nothing to do with #21? I think you've accidentally included some stuff that was submitted in another PR.
Also, would you mind adding a test for this fix? After that I think we can merge this.
| var reason = null; | ||
|
|
||
| var failures = matchFailure.getRightmostFailures(); | ||
| for(var i = 0; i < failures.length; i++) { |
There was a problem hiding this comment.
Nit: Missing space before (.
| var failure = failures[i]; | ||
| if (failure.getPExpr().ruleName === 'escapeChar'){ | ||
| // Failure to match an escape sequence, therefore the grammar contains an invalid escape sequence. | ||
| var escapeSequence = source.substr(matchFailure.getRightmostFailurePosition(),2); |
There was a problem hiding this comment.
Nit: Missing space after ,.
| if (failure.getPExpr().ruleName === 'escapeChar'){ | ||
| // Failure to match an escape sequence, therefore the grammar contains an invalid escape sequence. | ||
| var escapeSequence = source.substr(matchFailure.getRightmostFailurePosition(),2); | ||
| var reason = "Invalid escape sequence \"" + escapeSequence + "\""; |
There was a problem hiding this comment.
You shouldn't be re-declaring reason here -- should just be reason = ...
| } | ||
|
|
||
| if (reason) { | ||
| // Use the more relevant reason an the error message |
| var failure = failures[i]; | ||
| if (failure.getPExpr().ruleName === 'escapeChar'){ | ||
| // Failure to match an escape sequence, therefore the grammar contains an invalid escape sequence. | ||
| var escapeSequence = source.substr(matchFailure.getRightmostFailurePosition(),2); |
There was a problem hiding this comment.
Since you call matchFailure.getRightmostFailurePosition() several times here, why not declare var rightmostFailurePos = matchFailure.getRightmostFailurePosition(); above the for loop?
pdubroy
force-pushed
the
main
branch
4 times, most recently
from
ac91299
to
fdddc68
Compare
A quick implementation of #21. This is accomplished by extending GrammarSyntaxError to provide more relevant messages than the default "expected" messages that Ohm gives. This is, in my opinion, the most appropriate place to put it, as this exception is only used for failure to create user grammars, and this keeps all ohm-specific parse failure messages separate from the rest of Ohm's logic.
I won't lie, I'm not entirely satisfied with it quite yet, especially the changes I made to the visualizer to support taking this from the exception. Perhaps it might be better to integrate this elsewhere.