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.

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

Add stripIgnoredCharacters utility function #1802

Merged
merged 3 commits into from Apr 3, 2019

Conversation

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Mar 26, 2019

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

@IvanGoncharov IvanGoncharov changed the title Strip ignored characters Add stripIgnoredCharacters utility function Mar 26, 2019
@IvanGoncharov IvanGoncharov requested a review from mjmahone Mar 26, 2019
}
}

expectStripped(ignoredTokens.join('')).toEqual('');

This comment has been minimized.

Copy link
@IvanGoncharov

IvanGoncharov Mar 26, 2019

Author Member

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.

// 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', '\\'];

This comment has been minimized.

Copy link
@IvanGoncharov

IvanGoncharov Mar 26, 2019

Author Member

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

This comment has been minimized.

Copy link
Member Author

IvanGoncharov commented Mar 26, 2019

@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

This comment has been minimized.

Copy link

rybon commented Mar 26, 2019

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

@rybon
rybon approved these changes Mar 26, 2019
Copy link

rybon left a comment

Great work!

@IvanGoncharov IvanGoncharov force-pushed the APIs-guru:stripIgnoredCharacters branch 2 times, most recently from 18c66cc to 15441c9 Mar 26, 2019
Heavily based on work done by @rybon in #1628.
Solves #1523.
@IvanGoncharov IvanGoncharov force-pushed the APIs-guru:stripIgnoredCharacters branch from 15441c9 to 328b065 Mar 26, 2019
@codecov-io

This comment was marked as resolved.

Copy link

codecov-io commented Mar 26, 2019

Codecov Report

Merging #1802 into master will increase coverage by 1.16%.
The diff coverage is 99.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1802      +/-   ##
==========================================
+ Coverage   97.57%   98.73%   +1.16%     
==========================================
  Files         214      216       +2     
  Lines       13296    13649     +353     
  Branches     1965     1999      +34     
==========================================
+ Hits        12973    13477     +504     
- Misses        171      172       +1     
+ Partials      152        0     -152
Impacted Files Coverage Δ
src/utilities/stripIgnoredCharacters.js 100% <100%> (ø)
src/language/lexer.js 100% <100%> (ø) ⬆️
src/language/__tests__/lexer-test.js 100% <100%> (ø) ⬆️
src/language/__tests__/blockString-test.js 100% <100%> (ø) ⬆️
src/language/blockString.js 100% <100%> (ø) ⬆️
...utilities/__tests__/stripIgnoredCharacters-test.js 99.53% <99.53%> (ø)
src/jsutils/suggestionList.js 100% <0%> (ø) ⬆️
src/utilities/separateOperations.js 100% <0%> (ø) ⬆️
...alidation/__tests__/PossibleTypeExtensions-test.js 100% <0%> (ø) ⬆️
src/validation/rules/FieldsOnCorrectType.js 100% <0%> (ø) ⬆️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b5e955...328b065. Read the comment docs.

Copy link
Contributor

mjmahone left a comment

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

This comment has been minimized.

Copy link

rybon commented Apr 3, 2019

Merge?

@IvanGoncharov IvanGoncharov merged commit 081db43 into graphql:master Apr 3, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@IvanGoncharov IvanGoncharov deleted the APIs-guru:stripIgnoredCharacters branch Apr 3, 2019
@IvanGoncharov

This comment has been minimized.

Copy link
Member Author

IvanGoncharov commented Apr 3, 2019

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

@rybon

This comment has been minimized.

Copy link

rybon commented Apr 18, 2019

@IvanGoncharov could you do a release to NPM?

@IvanGoncharov

This comment has been minimized.

Copy link
Member Author

IvanGoncharov commented Apr 18, 2019

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

@IvanGoncharov

This comment has been minimized.

Copy link
Member Author

IvanGoncharov commented May 7, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.