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

Multi source reader for parsing graphql Documents #1411

Merged
merged 3 commits into from Feb 7, 2019

Conversation

bbakerman
Copy link
Member

This creates a multi source reader that can track which "source part" text comes from and map back to those places relative to each part.

The idea is that multiple files could feed a parsing document and errors reported in terms of those files not the logical place

Of course it works for a single string and presents 1 source and nothing will be relative

… text comes from and map back to those places relative to each part.

The idea is that multiple files could feed a parsing document and errors reported in terms of those files not the logical place

Of course it works for a single string and presents 1 source and nothing will be relative
@@ -139,7 +139,7 @@ class GraphQLTest extends Specification {
then:
errors.size() == 1
errors[0].errorType == ErrorType.InvalidSyntax
errors[0].locations == [new SourceLocation(1, 8)]
errors[0].locations == [new SourceLocation(0, 8)]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

{ hello } is now considered as 0 based not 1's based

Copy link
Member

Choose a reason for hiding this comment

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

why did you change the index? the spec says

where each location is a map with the keys line and column, both positive numbers starting from 1 which describe the beginning of an associated syntax element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh I did not know the spec says that - I guess we need to add one,

Ones based index are weird right?? It goes against 50 years of computing theory! ;)

Copy link
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.

Looks good overall ... please see my comments

return new Builder();
}

public static class Builder {
Copy link
Member

Choose a reason for hiding this comment

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

you want a private constructor here maybe

@@ -139,7 +139,7 @@ class GraphQLTest extends Specification {
then:
errors.size() == 1
errors[0].errorType == ErrorType.InvalidSyntax
errors[0].locations == [new SourceLocation(1, 8)]
errors[0].locations == [new SourceLocation(0, 8)]
}

Copy link
Member

Choose a reason for hiding this comment

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

why did you change the index? the spec says

where each location is a map with the keys line and column, both positive numbers starting from 1 which describe the beginning of an associated syntax element.


import spock.lang.Specification

class MultiSourceReaderTest extends Specification {
Copy link
Member

Choose a reason for hiding this comment

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

we should have a test for sourceName being null

Copy link

@tsroka tsroka left a comment

Choose a reason for hiding this comment

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

LGTM. Definitely will improve error reporting for multiple files!

@bbakerman bbakerman merged commit e310dc3 into graphql-java:master Feb 7, 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.

None yet

3 participants