Issue #43 - Customizable operation parse error #174

kyuweftea · 2016-11-13T21:55:07Z

pdubroy

Thanks for the PR! Looks good, I've just left a few suggestions that I think will improve the code a bit.

Also, to demonstrate that you've fixed the issue describe in #43, how about adding a test to test-errors.js?

pdubroy · 2016-11-14T14:44:34Z

src/MatchResult.js

@@ -171,6  171,31 @@ MatchFailure.prototype.getRightmostFailures = function() {
  return this._failures;
 };

+MatchFailure.prototype.getMessage = function(config) {
+  var source = this.state.inputStream.source;
+  if (typeof source !== 'string') {


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.

pdubroy · 2016-11-14T14:50:08Z

src/MatchResult.js

@@ -171,6  171,31 @@ MatchFailure.prototype.getRightmostFailures = function() {
  return this._failures;
 };

+MatchFailure.prototype.getMessage = function(config) {


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

pdubroy · 2016-11-14T14:50:12Z

src/MatchResult.js

+  var detail = 'Expected '   this.getExpectedText();
+
+  // use default values if not defined
+  var includeSource = typeof config !== 'undefined' &&


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;

pdubroy · 2016-11-14T14:50:58Z

src/Semantics.js

@@ -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}));


Don't we want includeLineNumbers to be false here?

pdubroy · 2016-11-14T14:54:51Z

src/util.js

@@ -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 */) {


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

pdubroy · 2016-11-14T14:58:21Z

src/util.js

+
+exports.getOptionalLineAndColumnMessage = function(str, offset, includeLineNumbers /* ranges */) {
+
+  var _includeLineNumbers = typeof includeLineNumbers !== 'undefined' ? includeLineNumbers : true;


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.

pdubroy · 2016-11-14T15:05:48Z

src/util.js

@@ -146,3  153,9 @@ exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {
  }
  return sb.contents();
 };
+
+exports.getLineAndColumnMessage = function(str, offset /* ...ranges */) {


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.

…added test

…eAndColumnMessage

… that relied on old method

kyuweftea · 2016-11-18T01:17:56Z

@pdubroy Suggestions were applied. Please review.

pdubroy

Looking good, just a couple of small changes and it's ready to merge.

pdubroy · 2016-11-18T16:02:39Z

test/test-util.js

@@ -90,49  91,49 @@ test('getLineAndColumnMessage', function(t) {
 });

 test('getLineAndColumnMessage with ranges', function(t) {


We should rename this test now, since it's not testing getLineAndColumnMessage anymore :-)

pdubroy · 2016-11-18T16:04:39Z

test/test-errors.js

+    t.fail('Expected an exception to be thrown');
+  } catch (e) {
+    t.equal(e.message, [
+      'Line 1, col 17:',


It doesn't appear that this test passes.

kyuweftea and others added 3 commits November 12, 2016 18:36

initial issue 43 commit

8eb972d

implmented backward compatibility for ranges

32f9c28

fixed caret position

a08e75c

pdubroy requested changes Nov 14, 2016

View reviewed changes

kyuweftea and others added 9 commits November 16, 2016 07:32

initial issue 43 commit

973d30f

implmented backward compatibility for ranges

f312eb5

fixed caret position

7ae7e2a

Fixing issues brought up in pull request comments, and added test

03a529f

Merge branch - fixed issues brought up in pull request comments, and …

4697e24

…added test

Merge branch - fixed issues brought up in pull request comments, and …

2a6d401

…added test

Merge branch - fixed tests that referenced previous version of getLin…

e8f8d07

…eAndColumnMessage

Fixing issues brought up in pull request comments, and added test

aa62353

Merge branch - fixed issue in commit with added test, and fixed tests…

a1d2b91

… that relied on old method

pdubroy requested changes Nov 18, 2016

View reviewed changes

pdubroy force-pushed the main branch 4 times, most recently from ac91299 to fdddc68 Compare March 4, 2023 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #43 - Customizable operation parse error #174

Issue #43 - Customizable operation parse error #174

kyuweftea commented Nov 13, 2016

pdubroy left a comment

pdubroy Nov 14, 2016

pdubroy Nov 14, 2016

pdubroy Nov 14, 2016

pdubroy Nov 14, 2016

pdubroy Nov 14, 2016

pdubroy Nov 14, 2016

pdubroy Nov 14, 2016

kyuweftea commented Nov 18, 2016

pdubroy left a comment

pdubroy Nov 18, 2016

pdubroy Nov 18, 2016


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

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

		@@ -90,49 91,49 @@ test('getLineAndColumnMessage', function(t) {
		});

		test('getLineAndColumnMessage with ranges', function(t) {

Issue #43 - Customizable operation parse error #174

Are you sure you want to change the base?

Issue #43 - Customizable operation parse error #174

Conversation

kyuweftea commented Nov 13, 2016

pdubroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyuweftea commented Nov 18, 2016

pdubroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment