Fix printed schema descriptions that end with a double quote (")#1205
Fix printed schema descriptions that end with a double quote (")#1205sethtippetts wants to merge 4 commits intographql:masterfrom
Conversation
|
cc @IvanGoncharov - I know you had a similar fix land earlier. Care to review this? |
@leebyron @mostcowbell Yes I fixed similar issue with Adding space changes description string and if we want to omit it than possible solution would be: Or: Also, you need to handle the same edge case as On the other hand descriptionLines already changes description string. Personally, I think |
|
@IvanGoncharov so I should add another assertion to my test? expect(output).to.equal(`
schema {
query: Root
}
type Root {
"""This field is "awesome" """
onlyField: String
}
`);or just a comment about the output of a description ending in a double-quote? // Will be printed as `"""This field is "awesome" """` to avoid parsing issues |
|
@mostcowbell I would suggest being as explicit as possible: const description = 'This field is "awesome"';
const output = printSingleFieldSchema({ type: GraphQLString, description });
expect(output).to.equal(`
...
`);
const recreatedRoot = buildSchema(output).getTypeMap()['Root'];
const recreatedField = recreatedRoot.getFields()['singleField'];
/* Note: Additional space added to prevent parser confusing trailing double
* quote inside description with the end of the block string. It shouldn't
* affect result since descriptions are interpreted as Markdown so trailing
* spaces are ignored.
*/
expect(recreatedField.description).to.equal(description + ' ');Disclaimer: I'm not a native speaker :)
And change test description to something like: |
|
@IvanGoncharov I've updated my test and removed the extra dependencies I was importing into that file |
| const Schema = new GraphQLSchema({ query: Root }); | ||
| const output = printSchema(Schema); | ||
| expect(() => parse(output)).not.to.throw(GraphQLError); | ||
| }); |
There was a problem hiding this comment.
@mostcowbell It's better to reuse printSingleFieldSchema similar to other tests since it allows you test that printSchema output not only parse-ready but you can recreate schema based on it. I also think it's important to have the output of printSchema inside test so you would see how doublequote is escaped. So I suggest to change it like this:
const description = 'This field is "awesome"';
const output = printSingleFieldSchema({ type: GraphQLString, description });
expect(output).to.equal(dedent`
schema {
query: Root
}
type Root {
"""This field is "awesome" """
singleField: String
}
`);I experiment a bit and expend test to ensure that description stays unchanged:
const recreatedDescription = buildSchema(output)
.getTypeMap()['Root']
.getFields()['singleField']
.description;
expect(recreatedDescription).to.equal(description);And it fails 😞
AssertionError: expected 'This field is "awesome" ' to equal 'This field is "awesome"'
^ Note space here
There was a problem hiding this comment.
After giving it a thought I don't think that adding space to a description is such a big issue but it should be explicitly documented in the test, like:
expect(recreatedDescription).to.equal(description + ' ');
And since it's not so obvious issue, both test and implementation should contain a comment with an explanation for why it's needed.
|
I'd like to see a solution that also handles the edge cases you referenced, @IvanGoncharov - I'll pull in these test cases and ensure the solution accounts for them. |
4 participants
Schemas printed with
printSchemathat contain descriptions ending with a double-quote character are not parseable and throw aGraphQLError: Syntax Error: Unterminated stringbecause the SDL description ends with 4 contiguous double-quotesI've included a failing test case: