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

Clean up clippy warnings remove try! #135

Merged
merged 5 commits into from
May 3, 2018

Conversation

Atul9
Copy link
Contributor

@Atul9 Atul9 commented Jan 25, 2018

closes #131

@Atul9 Atul9 force-pushed the clean_clippy_remove_try branch 2 times, most recently from 6fb8fba to a7d843e Compare January 27, 2018 02:33
@Atul9 Atul9 changed the title Clean up clippy warnings remove try! [WIP] Clean up clippy warnings remove try! Jan 27, 2018
@Atul9 Atul9 force-pushed the clean_clippy_remove_try branch 2 times, most recently from eb110fc to 438d56a Compare January 28, 2018 07:55
@Atul9 Atul9 changed the title [WIP] Clean up clippy warnings remove try! Clean up clippy warnings remove try! Jan 28, 2018
@theduke
Copy link
Member

theduke commented Feb 5, 2018

@Atul9 do you need help with figuring out the build failures?

@Atul9
Copy link
Contributor Author

Atul9 commented Feb 6, 2018

@theduke Yes, I need help with figuring out the build failures. I had mentioned this on the issue #131 (comment)

@neoeinstein
Copy link
Contributor

@Atul9 I added a PR to Atul9#1 which should correct the build errors.

Atul9 and others added 5 commits May 3, 2018 01:50
It doesn't appear that `:tt` accepts the `stringify!()`-ed value in this
position. The :tt is only later used as an `:expr` to produce the name
for metadata purposes.

Converting this position to be an `:expr` allows the `stringify!()`-ed
value and accepts all current uses of the `graphql_scalar!()` macro in
this repository.
The conversions in this changeset cannot use the `From<T>` trait
implementation because the conversion is lossy, either because they
involve converting a signed value to an unsigned value (`i32`⇒`u64`)
or because the convert from a larger data type to a smaller one
(`u64`⇒`i32`). In this case, the `as` type cast is necessary to perform
a bitwise conversion.

This coercion can cause negative values to become very large unsigned
values. This is intentional on line 90.
The `E::custom()` function requires a value of any type that implements
`fmt::Display`, so a plain `&str` works just fine here.
@codecov-io
Copy link

Codecov Report

Merging #135 into master will increase coverage by 0.14%.
The diff coverage is 74.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   90.24%   90.39%   +0.14%     
==========================================
  Files          95       95              
  Lines       16429    16913     +484     
==========================================
+ Hits        14827    15289     +462     
- Misses       1602     1624      +22
Impacted Files Coverage Δ
juniper/src/macros/scalar.rs 100% <ø> (ø) ⬆️
juniper/src/schema/model.rs 92.33% <ø> (+2.87%) ⬆️
juniper/src/executor_tests/variables.rs 96.88% <0%> (+1.53%) ⬆️
juniper/src/http/mod.rs 0% <0%> (ø) ⬆️
...r/src/executor_tests/introspection/input_object.rs 94.27% <0%> (-0.81%) ⬇️
juniper/src/types/name.rs 80.85% <100%> (ø) ⬆️
juniper/src/lib.rs 84.61% <100%> (ø) ⬆️
juniper/src/parser/document.rs 99.15% <100%> (+1.41%) ⬆️
juniper/src/ast.rs 77.41% <100%> (ø) ⬆️
juniper/src/parser/lexer.rs 92.15% <100%> (ø) ⬆️
... and 17 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 05c1011...5fad1c4. Read the comment docs.

@theduke
Copy link
Member

theduke commented May 3, 2018

@Atul9 @neoeinstein thanks a bunch to you both, that was quite a bite of work.

@theduke theduke merged commit b94ed37 into graphql-rust:master May 3, 2018
@theduke
Copy link
Member

theduke commented May 3, 2018

@neoeinstein also thanks for the detailed commit messages. ;)

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

Successfully merging this pull request may close these issues.

Clean up Clippy warnings + remove try!
4 participants