Skip to content
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

Issue #43 - Customizable operation parse error #174

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
179 changes: 118 additions & 61 deletions dist/ohm.js

Large diffs are not rendered by default.

10 changes: 5 additions & 5 deletions dist/ohm.min.js

Large diffs are not rendered by default.

25 changes: 25 additions & 0 deletions src/MatchResult.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,31 @@ MatchFailure.prototype.getRightmostFailures = function() {
return this._failures;
};

MatchFailure.prototype.getMessage = function(config) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a short comment above this function explaining what options can be passed in the config arg.

var source = this.state.inputStream.source;
if (typeof source !== 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this if. If you copied this from somewhere else, it's left over from a time when we matching on arrays as well as strings. Now, we just support strings, so there's no need to check.

return 'match failed at position ' + this.getRightmostFailurePosition();
}
var detail = 'Expected ' + this.getExpectedText();

// use default values if not defined
var includeSource = typeof config !== 'undefined' &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a cleaner to do this slightly differently. First, rename the argument optConfig, and then, at the top of the function, add the following:

var config = optConfig || {};

Then, define includeSource and includeLineNumbers like so:

var includeLineNumbers = config.includeLineNumbers == null ? true : config.includeLineNumbers;
var includeSource = config.includeSource == null ? true : config.includeSource;

typeof config.includeSource !== 'undefined' ?
config.includeSource : true;
var includeLineNumbers = typeof config !== 'undefined' &&
typeof config.includeLineNumbers !== 'undefined' ?
config.includeLineNumbers : true;

if (!includeSource) {
return getShortMatchErrorMessage(
this.getRightmostFailurePosition(),
this.state.inputStream.source,
detail);
}
return util.getOptionalLineAndColumnMessage(
source, this.getRightmostFailurePosition(), includeLineNumbers) + detail;
};

// Return a string summarizing the expected contents of the input stream when
// the match failure occurred.
MatchFailure.prototype.getExpectedText = function() {
Expand Down
2 changes: 1 addition & 1 deletion src/Semantics.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ function parseSignature(signature, type) {
signature,
type === 'operation' ? 'OperationSignature' : 'AttributeSignature');
if (r.failed()) {
throw new Error(r.message);
throw new Error(r.getMessage({includeSource: true, includeLineNumbers: true}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want includeLineNumbers to be false here?

}

return prototypeGrammarSemantics(r).parse();
Expand Down
21 changes: 17 additions & 4 deletions src/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,10 @@ exports.getLineAndColumn = function(str, offset) {

// Return a nicely-formatted string describing the line and column for the
// given offset in `str`.
exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {

exports.getOptionalLineAndColumnMessage = function(str, offset, includeLineNumbers /* ranges */) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the line numbers are optional, I think we could pick a better name for this. Maybe 'describeSourceLocation'?


var _includeLineNumbers = typeof includeLineNumbers !== 'undefined' ? includeLineNumbers : true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it's unusual for local variables to start with _. Instead, I'd name the argument optIncludeLineNumbers and then name this variable includeLineNumbers. Or, to simplify things, we could just make the argument required.

var repeatStr = common.repeatStr;

var lineAndCol = exports.getLineAndColumn(str, offset);
Expand All @@ -109,7 +112,11 @@ exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {

// Helper for appending formatting input lines to the buffer.
function appendLine(num, content, prefix) {
sb.append(prefix + lineNumbers[num] + ' | ' + content + '\n');
if (_includeLineNumbers) {
sb.append(prefix + lineNumbers[num] + ' | ' + content + '\n');
} else {
sb.append(' ' + content + '\n');
}
}

// Include the previous line for context if possible.
Expand All @@ -123,7 +130,7 @@ exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {
// Start with a blank line, and indicate each range by overlaying a string of `~` chars.
var lineLen = lineAndCol.line.length;
var indicationLine = repeatStr(' ', lineLen + 1);
var ranges = Array.prototype.slice.call(arguments, 2);
var ranges = Array.prototype.slice.call(arguments, 3);
for (var i = 0; i < ranges.length; ++i) {
var startIdx = ranges[i][0];
var endIdx = ranges[i][1];
Expand All @@ -135,7 +142,7 @@ exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {

indicationLine = strcpy(indicationLine, repeatStr('~', endIdx - startIdx), startIdx);
}
var gutterWidth = 2 + lineNumbers[1].length + 3;
var gutterWidth = _includeLineNumbers ? 2 + lineNumbers[1].length + 3 : 2;
sb.append(repeatStr(' ', gutterWidth));
indicationLine = strcpy(indicationLine, '^', lineAndCol.colNum - 1);
sb.append(indicationLine.replace(/ +$/, '') + '\n');
Expand All @@ -146,3 +153,9 @@ exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {
}
return sb.contents();
};

exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd get rid of the optional ranges here. This function can be the simple one that includes the line number and only points to a single offset, and any code that needs more than that can call the other version (which I've suggested you rename to describeSourceLocation.

var args = Array.prototype.slice.call(arguments);
args.splice(2, 0, true);
return exports.getOptionalLineAndColumnMessage.apply(this, args);
};