-
Notifications
You must be signed in to change notification settings - Fork 426
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
Upgrade juniper_codegen dependencies #231
Conversation
Codecov Report
@@ Coverage Diff @@
## master #231 +/- ##
=========================================
- Coverage 89.88% 87.29% -2.6%
=========================================
Files 95 96 +1
Lines 17945 13943 -4002
=========================================
- Hits 16130 12171 -3959
+ Misses 1815 1772 -43
Continue to review full report at Codecov.
|
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.
For all these extend()
, can you change them to TokenStream::from()
? The old code assigned/replaced and the new code extends/appends. While currently they end up being equivalent, let's preserve the old semantics.
Thanks for this PR! I was dragging my feat on this and love that you came up with a PR. 🎉 |
So I changed the initial upgrade commit to keep the structure as before (because you're correct that is the minimal change), but then also added a commit that changes the code to the (simpler, in my mind) direct extend structure again. Feel free to skip that commit if you disagree it's an improvement. Also fixed some typos I found during the fixes. |
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.
Thanks! New changes look good to me.
It looks like due to the regex
bump CI fails on Rust 1.21. I think it is worth getting to regex 1.0 and we still work on Rust 1.22 which was released a year ago...so I'm fine dropping 1.21 for this.
The choice is up to you: remove the regex
bump from this PR or remove 1.21 from CI, add 1.23, and add a note to the README and CHANGELOG about new minimum rust requirement.
Breaks with the newer dependencies on this branch.
This foregoes the temporary variables and extends the TokenStream directly, preventing potentially confusing use of singular/plural variable names.
Replaced 1.21 with 1.23 for Travis CI and added a note to the changelog about it. I couldn't find any mention of the minimum supported Rust version in README, so I left that for now -- would be happy to do a follow-up, though. |
No description provided.