-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improve dedent (fixes #1202) #1203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me - I agree that being explicit is better, though I prefer the String.raw
approach discussed to avoid making the test cases harder to read.
@@ -190,7 +190,7 @@ describe('Printer: Query document', () => { | |||
|
|||
fragment frag on Friend { | |||
foo(size: $size, bar: $b, obj: {key: "value", block: """ | |||
block string uses \""" | |||
block string uses \\""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing this to \\
, could you use String.raw
as discussed in #1202? This test case intends to show the actual GraphQL string written, so avoiding double-encoding is intentional.
@@ -212,7 +212,7 @@ describe('Type System Printer', () => { | |||
} | |||
|
|||
type Root { | |||
singleField(argOne: String = "tes\t de\fault"): String | |||
singleField(argOne: String = "tes\\t de\\fault"): String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
That's true, using String.raw is clearer for the printer tests. I changed it and added another unit test to make sure interpolation also works properly when dedent is used as a string tag. |
src/jsutils/__tests__/dedent-test.js
Outdated
`).to.equal( | ||
'type Query {\n me: User\n}\n\n' + | ||
'type User {\n id: ID\n name: String\n}\n', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current form, these tests are hard to read.
Would be great to have test string in a separate variable and expected string in readable form, e.g.:
const output = dedent`
type Query {
me: User
}
`;
expect(output).to.equal([
'type Query {',
' me: User',
'}',
].join('\n'));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that looks a bit nicer, particularly if you're using that style already elsewhere. I refactored it already.
Changed the behavior of the function to not have the undocumented side effect of converting the string to its escaped (raw) form any more, and to also consider tabs in addition to spaces. Also added unit tests for the dedent function and adapted existing tests that were depending on the old behavior of the function.
Rebased onto current master now. |
As discussed in #1202, this changes the behavior of the dedent function that is used in unit tests to not return raw strings any more. Also added tests for the function itself.