Skip to content

Conversation

@haikusw
Copy link
Contributor

@haikusw haikusw commented Aug 26, 2020

This PR addresses #64 "GraphQL doesn't support 'Blockstrings' which are often used in Descriptions".

There are multiple ways to implement this support and I tried to pick one that matches the existing style of lexing/parsing. Lots of tests are included, though I'm sure more could be thought of.

There's one piece that I just noticed is missing: support for escaped Blockstrings \"""
I'm out of time for this project and don't actually need those (yet), so I'm going to punt on that for now. Shouldn't be terribly difficult, but… ¯_(ツ)_/¯.

I hope you find these contributions acceptable in form and substance. Feel free to let me know if there's things you'd prefer to be done differently.

All my work has been done on macOS 10.15.6 using Xcode 12b5, so I will be interested to see if this builds on linux.

Fixes #64

First pass at block string support.  Not sufficient

This doesn’t trim leading indentation (or trailing whitespace) from the lines inside the block string, which is expected and the specific requirements are detailed in the graphql spec.

Added the algorithm/notes from the spec and a blockStringValue() empty function to implement this in.

Saving this old work because it works other than that.
…tests.

add tests for empty string and blockstring.
take out trimming of string since that is moving to blockStringValue(rawString:) next.
update comment for readString
this commit has the debugging code still in it. next one will remove that.
…ings

subtle: swift .split omits blank lines by default.
But other issues combined with that.
@haikusw
Copy link
Contributor Author

haikusw commented Aug 26, 2020

oi. Forgot I was going to go back and try to clean up the test function names…

@haikusw haikusw closed this Aug 26, 2020
@haikusw haikusw reopened this Aug 26, 2020
@paulofaria
Copy link
Member

Hello, @haikusw. Thank you very much for your contribution! Just so you know, the structure of this repo mainly mirrors the structure of the javascript canonical GraphQL implementation. For your next contributions would be valuable to check how the js counterpart implements what you're after. For example:

https://github.com/graphql/graphql-js/blob/fe3040fffe7f34c8f8b8e1cb849f09138debbfab/src/language/lexer.js#L526

Most of the times a direct "translation" from Typescript do Swift is possible. 🙂

@paulofaria paulofaria merged commit e8f3c95 into GraphQLSwift:master Aug 26, 2020
@haikusw
Copy link
Contributor Author

haikusw commented Aug 26, 2020

Hello, @haikusw. Thank you very much for your contribution! Just so you know, the structure of this repo mainly mirrors the structure of the javascript canonical GraphQL implementation. For your next contributions would be valuable to check how the js counterpart implements what you're after. For example:

https://github.com/graphql/graphql-js/blob/fe3040fffe7f34c8f8b8e1cb849f09138debbfab/src/language/lexer.js#L526

Most of the times a direct "translation" from Typescript do Swift is possible. 🙂

Ah, I did not realize this. That does sound like it might have saved some time. Thank you.

@haikusw
Copy link
Contributor Author

haikusw commented Aug 26, 2020

Hello, @haikusw. Thank you very much for your contribution! Just so you know, the structure of this repo mainly mirrors the structure of the javascript canonical GraphQL implementation. For your next contributions would be valuable to check how the js counterpart implements what you're after. For example:
https://github.com/graphql/graphql-js/blob/fe3040fffe7f34c8f8b8e1cb849f09138debbfab/src/language/lexer.js#L526
Most of the times a direct "translation" from Typescript do Swift is possible. 🙂

Ah, I did not realize this. That does sound like it might have saved some time. Thank you.

I see that they did what I did initially which was to make a new .blockstring token type. I ended up backing that out because it would have required a lot of checking for either .string or .blockstring token types for things like description where it can be either. Seems like ideally I would go back and make this set of changes match the canonical GraphQL implementation since that is the essence of what this is. I will try and do that.

@paulofaria
Copy link
Member

Seems like ideally I would go back and make this set of changes match the canonical GraphQL implementation since that is the essence of what this is. I will try and do that.

That sounds great!

@haikusw
Copy link
Contributor Author

haikusw commented Aug 27, 2020

done: #68

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.

GraphQL doesn't support "Blockstrings" which are often used in Descriptions.

2 participants