Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add stripIgnoredCharacters utility function #1802

Merged
merged 3 commits into from
Apr 3, 2019
Merged

Add stripIgnoredCharacters utility function #1802

merged 3 commits into from
Apr 3, 2019

Conversation

IvanGoncharov
Copy link
Member

Heavily based on work done by @rybon in #1628.
Fixes #1523.

@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Mar 26, 2019
@IvanGoncharov IvanGoncharov changed the title Strip ignored characters Add stripIgnoredCharacters utility function Mar 26, 2019
@IvanGoncharov IvanGoncharov requested a review from mjmahone March 26, 2019 13:08
IvanGoncharov
IvanGoncharov commented Mar 26, 2019
View reviewed changes
src/utilities/__tests__/stripIgnoredCharacters-test.js
}
}

expectStripped(ignoredTokens.join('')).toEqual('');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote tests as a combination of handwritten test that covers algorithm and all branches and fuzzing tests that test all possible combinations of tokens.
Fuzzing tests helped to uncover a few edge cases that I fixed and include in handwritten tests but I think fuzzing tests are still useful in case if we change the code or extend GraphQL language grammar.

IvanGoncharov
IvanGoncharov commented Mar 26, 2019
View reviewed changes
src/utilities/__tests__/stripIgnoredCharacters-test.js
// Testing with length >5 is taking exponentially more time however it
// highly recommended to test with increased limit if you make any change
const maxCombinationLenght = 5;
const possibleChars = ['\n', ' ', '"', 'a', '\\'];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test helped me to uncovered that

"""
a
 a
"""

can't be converted into:

"""a
 a"""

Since you will change string value from a\n a into a\na.
That's why I think it's important to keep it even if can't run it with length >5 by default.

@IvanGoncharov
Copy link
Member Author

@rybon Sorry for the delay, it took me significantly more time than I expected.
Since some developers want to use this function as part of the hashing algorithm for functions I wanted this function to be well tested and handle all edge cases.
Can you please review this implementation to see if it meets your requirements?

@Cito @martijnwalraven @mjmahone If you have time, can you please review this PR?

@rybon
Copy link

rybon commented Mar 26, 2019

@IvanGoncharov yes, I will review it now. Thanks again!

@rybon rybon mentioned this pull request Mar 26, 2019
Closed
@IvanGoncharov IvanGoncharov force-pushed the stripIgnoredCharacters branch from 7c07691 to a9862a3 Compare March 26, 2019 13:35
rybon
rybon approved these changes Mar 26, 2019
View reviewed changes
Copy link

@rybon rybon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@IvanGoncharov IvanGoncharov force-pushed the stripIgnoredCharacters branch 2 times, most recently from 18c66cc to 15441c9 Compare March 26, 2019 13:46
@IvanGoncharov IvanGoncharov force-pushed the stripIgnoredCharacters branch from 15441c9 to 328b065 Compare March 26, 2019 13:50
@codecov-io

This comment has been minimized.

Sign in to view
mjmahone
mjmahone approved these changes Mar 26, 2019
View reviewed changes
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an elegant solution to me: use the lexer to condense the tokens, and then just print the tokens, rather than going through the lex => parse => print indirection.

@rybon
Copy link

rybon commented Apr 3, 2019

Merge?

@IvanGoncharov IvanGoncharov merged commit 081db43 into graphql:master Apr 3, 2019
@IvanGoncharov IvanGoncharov deleted the stripIgnoredCharacters branch April 3, 2019 20:38
@IvanGoncharov
Copy link
Member Author

@rybon Merged 🎉
I will work on preparing a release next week.

@rybon
Copy link

rybon commented Apr 18, 2019

@IvanGoncharov could you do a release to NPM?

@IvanGoncharov
Copy link
Member Author

@rybon Just returned from GraphQL Asia.
And new release is at the top of the list, I will try to release tomorrow.

@IvanGoncharov
Copy link
Member Author

@rybon Sorry for the delay.
I just published 14.3.0 📦 that includes this PR 🎉

@snyk-bot snyk-bot mentioned this pull request Aug 31, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Sep 11, 2019
Open
@dhinklexo dhinklexo mentioned this pull request Jul 16, 2020
Closed
@glasser glasser mentioned this pull request Mar 30, 2021
Merged
@spawnia spawnia mentioned this pull request Dec 7, 2021
Open
@hixus hixus mentioned this pull request Jan 26, 2022
Closed
@jerelmiller jerelmiller mentioned this pull request Apr 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rybon rybon rybon approved these changes

@mjmahone mjmahone mjmahone approved these changes

Assignees
No one assigned
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: strip whitespace from GraphQL queries / fragments / mutations / subscriptions
4 participants
@IvanGoncharov @rybon @codecov-io @mjmahone