Parse string based descriptions #269
Conversation
|
Thank you for your PR! I'll review it shortly! |
|
Thinking about this PR again this morning, I realize it's a real quick and dirty fix that works for my current use case, but in general is way too permissive. I'm thinking of improving it by splitting up Consume() from ConsumeDescription() instead of having a flag passed into the same function that consumes either one. That way I can significantly reduce the possible invalid schemas accepted. I think I was trying too hard to keep my changes only within the Consume() function and that led to a bad design. Feel free to review other PRs first. I'll let you know when I think this is in a mergable state. |
gracenoah
changed the title
gracenoah
force-pushed
the
descriptions
branch
from
da649f5 to
d79e178
Compare
gracenoah
changed the title
|
Updated. I think this is a bit more reasonable. I'm still not happy with the level of testing, but it did work with the new flag on in our codebase. |
graphql.go
Outdated
| } | ||
|
|
||
| if err := s.schema.Parse(schemaString); err != nil { | ||
| if err := s.schema.Parse(schemaString, s.noCommentsAsDescriptions); err != nil { |
There was a problem hiding this comment.
Should we name the variable useStringComments or some other shorter name instead? I think that shorter is better. WDYT?
There was a problem hiding this comment.
I'll rename it to useStringsAsDescriptions. The spec uses the term "description" so I prefer to use that here too.
There was a problem hiding this comment.
Actually, useStringDescriptions is as short as I can make it.
|
Is there anything preventing this from being merged? It'd be great to be able to load schemas with documentation in them. |
|
@Arachnid I'm sorry for the delay. I'm currently looking into subscriptions and then I'll get back to the remaining PRs - I think that tis name (i.e. |
|
Ready to merge if you are happy with it. |
internal/common/lexer.go
Outdated
| l.descComment += "\n" | ||
| } | ||
|
|
||
| comment := "" |
There was a problem hiding this comment.
I would prefer using the default value here instead of using an assignment:
var comment string
|
Sorry I haven't been responsive. I think I'll be able to get back to this next week. |
|
updated |
Parse string based descriptions
3 participants
This implementation is probably lax about where string descriptions are allowed. Additional test cases would be needed to verify the accuracy of the implementation. Also, this is backwards compatible with comment based descriptions and there's an option to disable the backwards compatibility.
fix #196
fix #147