Skip to content

The line numbers of elements are no longer off by one#1513

Merged
bbakerman merged 1 commit intographql-java:masterfrom
bbakerman:1512-off-by-one-errors
May 1, 2019
Merged

The line numbers of elements are no longer off by one#1513
bbakerman merged 1 commit intographql-java:masterfrom
bbakerman:1512-off-by-one-errors

Conversation

@bbakerman
Copy link
Copy Markdown
Member

see #1512

The previous multi source line reader work was a bit hasty in how it calculated line numbers.

Antler is 1s based while the multi source reader is zeroes based - so we got out by one

And then the tests got updated to reflect it - even though it was wrong in practice

This corrects that

@bbakerman bbakerman requested review from andimarek and tsroka April 26, 2019 07:00
@bbakerman bbakerman added this to the 13.0 milestone Apr 26, 2019
Copy link
Copy Markdown
Member

@andimarek andimarek left a comment

Choose a reason for hiding this comment

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

I think we should use create... instead of mk... ... my 2 cent :-)


class SourceLocationHelper {

static SourceLocation mkSourceLocation(MultiSourceReader multiSourceReader, Token token) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know we have it already at some other places, but I really prefer create... over mk.... I think it is just clearer.

@@ -36,12 +37,8 @@ public Token recoverInline(Parser recognizer) throws RecognitionException {
}

InvalidSyntaxException mkMoreTokensException(Token token) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see my other comment about mk... :-)

@bbakerman bbakerman merged commit 61d7c7b into graphql-java:master May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants