Closed
Conversation
Contributor
|
Thank you for this PR. Out of curiosity, does this address a particular issue you were encountering? |
Contributor
Author
|
it... sort of. i was seeing failure for parsing unicode. in fact there was no implementation yet so i went on to add minimal test for non-unicode cases. would be happy to do another pr for unicode support though. |
andimarek
requested changes
May 20, 2017
Member
andimarek
left a comment
There was a problem hiding this comment.
Thank you very much for this PR.
I have commented on some changes.
As an alternative you could give us the right to change the PR: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/
Thanks!
| public class GraphqlAntlrToLanguage extends GraphqlBaseVisitor<Void> { | ||
|
|
||
| Document result; | ||
| private Document result; |
Member
There was a problem hiding this comment.
Could you revert it? This change is unrelated to the parsing.
|
|
||
| import spock.lang.Ignore | ||
| import spock.lang.Specification | ||
| /** |
Member
There was a problem hiding this comment.
Could you remove this comment? We don't maintain author comments.
|
|
||
| private String parseString(String string) { | ||
| static String parseString(String string) { | ||
| assert string.length() >= 2; |
Member
There was a problem hiding this comment.
We don't use assert statements in prod code: they are turned off by default and don't have any effect.
Also it is not really needed to ensure the length or that the string starts wit ", this is already ensured by the parser/lexer.
bbakerman
added a commit
to bbakerman/graphql-java
that referenced
this pull request
May 25, 2017
Member
|
re implemented here : #464 |
Contributor
Author
|
@bbakerman been busy recently, thanks for taking over |
andimarek
pushed a commit
that referenced
this pull request
May 26, 2017
GrigoryPtashko
added a commit
to GrigoryPtashko/graphql-java
that referenced
this pull request
May 29, 2017
* upstream/master: graphql-java#438 - unit test for NonNullableFieldValidator graphql-java#438 now with double checks in null values that might come out of coercion graphql-java#457 - support for implicit schema when types are named `Query` graphql-java#427 TDD driven null support (graphql-java#452) refactoring add some comments fix javadoc tweak build update jdk version for travis build cleanup build javadoc too to ensure that it is all valid javadoc fix javadoc mark as internal cleanup mark internal validation classes as @internal a bit refactoring Added antlr parsing tests as outlined in graphql-java#200 graphql-java#448 parse null pointer make buildRegistry public Introspection parser (graphql-java#463) fix test improve assertion error document current behaviour of failed serialization with test rename test classes to *Test (before it was *Spec) update javadoc for serialize/parseValue fix test add MapEnumValuesProvider for simple map based mappings rename StaticEnumValuesProvider to NaturalEnumProvider enum values provider Scalar changes (graphql-java#455) cleanup: combine catch block Make NoOpInstrumentation.INSTANCE final don't trim comment lines for description bugfix: comments as descriptions this is a small bugfix for enum field descriptions and a change so that an empty line in an comment separate a comment from a description (for the IDL) adding tests for missing arguments to document the current behaviour add test for too large int literal, which is failing currently docs: fix styling issues docs docs docs doc: add relay info cleanup: remove assert statements and author comments remove jacoco: not working currently improve javadoc, mark as public spi docs: typo update version updated readme, delete readme.next initial version of new documentation improve assertion method refactoring: removed `ResolvedTypeInterface` graphql-java#419 - dynamic runtime wiring factory support add index.rst add docs folder Add environment to field instrumentation. Update README.next.md Move related projects to top of readme fix merge fix merge add lincol to exception Download gradle over https add public annotation add public annotation renames SchemaCompiler -> SchemaParser and SchemaDecompiler -> SchemaPrinter IDL: fix subscription support IDL: fix missing subscription support add public/internal annotations fix IDL example add test for SchemaValidator graphql-java#414 added schema validation in subscriptions remove link to google group: ask to open a new issue instead remove javadoc link 425 improve wrong type exceptions (graphql-java#429) phrase better exception messages fix union IDL parsing and add tests for it rebase from master better exceptions for incorrect types fix union type generation (test missing) do not overwrite top level schema definition during type registry merge Fixed tests Test class that had an invalid schema in place object interface is now validated added argument support to graphql schema checking on interfaces - with default value checks added argument support to graphql schema checking on interfaces remove list of related projects in favour of the awesome list add @documented add public/internal/spi annotations graphql-java#409 - renamed schema validator code to exactly that graphql-java#410 - Added interface checking on types at IDL level graphql-java#409 - Added interface checking on object types graphql-java#406 - PR clean code fix ups Made DataFetchingFieldSelectionSet a supplier since it really is Added a selection set interface instead as suggested. This means field collector doesnt NOT have to be API revert changes: will be handled in a PR add public/internal/spi annotations Documentation: add graphql language identifier Documentation: remove outdated java 8 lambdas note Documentation: use correct heading Documentation: minor wording change remove duplicate code remove TypeOrReference, but changing the return type of `getInterfaces` and `getTypes` at the same time. redefine references api Removes the TypeReference class in favour of the already existing GraphQLTypeReference. simplify subscription test a bit subscription documentation subscription documentation add subscription documentation graphql-java#406 readme updates for type extensions graphql-java#406 added extend type XXX {} support ExecutionContext is not longer API but there is a replacement Makes type name specified to builder graphql-java#402 - put a next version README in place graphql-java#377 - have the ability to know and capture all fields in a data fetcher cleanup: replace explicit type arguments with diamond operator <> cleanup: remove redundant type infos cleanup: rename class to match actual file remove BuilderFunction in favour of Java 8 UnaryOperator Merge remote-tracking branch 'upstream/master' into kaqqao-122TypeResolver cleanup: public is not needed on interfaces Added more tests for the bug in schema generator Fixed a bug where schema generator is nor respecting type uniqueness PR feedback - moved classes around and made the IDL spec support call PR feedback and made methods static since they had no state Breaking change: Renaming the map with extra types for the schema from `dictionary` to `additionalTypes` to make it more clear. Revert "graphql-java#383 added extra readme documentation for specific references" Add failing test case for missing fragment name Adding extra data to type resolver graphql-java#379 added extra readme documentation for causing mutations graphql-java#383 added extra readme documentation for specific references Add fragment definitions and execution id to DataFetchingEnvironment (fixes graphql-java#303). Fixed a problem where non unique type names cause class exceptions later down the track. Removes javadoc warnings during build time graphql-java#296 - now with an AST from object support and tests graphql-java#296 -more tests for extra conditions graphql-java#296 - added a AST pretty printer system so I can fix other problems like 296 Allow ExecutionStrategy to specify dataFetchingExceptionHandler graphql-java#381 - Added more tests for merging and change the signature to throw SchemaProblem revert two files graphql-java#381 - Adds the ability to compile and build executable schemas from schema IDL definitions and also method reference use lambda where possible graphql-java#352 - fixed the bug where too many tokens are allowed as valid closes graphql-java#122 Provide more information to TypeResolver fix with java8 type param and misc. Adding support for subscriptions with an implemenation to enable adding a user defined subscriptionType. Subscriptions can be thought of as mutations by a different name and with a different purpose and, as such, the implementation here is the same as that of mutations. graphql-java#268 - now follows spec in regard to null error handling fix an errant newline Check for invalid list indices on client-provided cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
adding parsing test in antlr parsing