Skip to content

Implement newtype derive for scalars. #368

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

Merged
merged 3 commits into from
Jun 25, 2019

Conversation

theduke
Copy link
Member

@theduke theduke commented May 26, 2019

This commit implements a newtype style custom derive
for scalars via #[derive(GraphQLScalarValue)], which now
supports both deriving a base enum scalar type and newtypes.

For newtypes, the #[graphql(transparent)] attribute is
required.

This commit:

  • implements the derive
  • adds integration tests
  • updates the book

Closes #303

@theduke theduke requested a review from LegNeato May 26, 2019 09:12
@theduke theduke force-pushed the scalar-derive-transparent branch 2 times, most recently from c4d8890 to 3d159f3 Compare May 26, 2019 09:29
@codecov-io
Copy link

codecov-io commented May 26, 2019

Codecov Report

Merging #368 into master will decrease coverage by 0.36%.
The diff coverage is 33.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
- Coverage   86.08%   85.72%   -0.37%     
==========================================
  Files         107      108       +1     
  Lines       15600    15710     +110     
==========================================
+ Hits        13429    13467      +38     
- Misses       2171     2243      +72
Impacted Files Coverage Δ
juniper_codegen/src/lib.rs 2.12% <ø> (ø) ⬆️
...s/juniper_tests/src/codegen/derive_input_object.rs 64.17% <ø> (ø) ⬆️
juniper_codegen/src/derive_scalar_value.rs 0% <0%> (ø) ⬆️
...iper_tests/src/codegen/scalar_value_transparent.rs 92.68% <92.68%> (ø)

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 a84dbf7...0268d03. Read the comment docs.

@theduke theduke changed the title (codegen) Implement newtype derive for scalars. Implement newtype derive for scalars. May 26, 2019
@theduke
Copy link
Member Author

theduke commented May 30, 2019

@LegNeato ping

@LegNeato
Copy link
Member

I haven't looked at this super closely, but why would you ever not want transparent for a newtype?

Can we do it automatically for newtypes like serde does?

@theduke
Copy link
Member Author

theduke commented May 30, 2019

Serde requires the #[serde(transparent)] attribute.

Technically we could just do newtype for tuple structs with a single filed, but since we are reusing the macro for two different purposes I prefer to be explicit. It also gives some more clarity as to what's happening for the reader of the code, and it's analogous to serde and #[repr(transparent)].

@LegNeato
Copy link
Member

But what about this from https://serde.rs/impl-serialize.html:

Data formats are encouraged to treat newtype structs as insignificant wrappers around the inner value, serializing just the inner value

And this link shows that the json serializer at least doesn't need serde(transparent):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d9d970167e8550e808b153252eb89eaf

@theduke
Copy link
Member Author

theduke commented May 31, 2019

I always thought the #transparent attr was mandatory.
Turns out serde also supports [#transparent] with regular structs with only one field, and tuple structs with multiple fields but only one that's not skipped.

Not so sure if we'd want to support that.

@LegNeato
Copy link
Member

I think we should mirror serde's behavior whenever possible...it's very ergonomic and battle-tested.

@LegNeato
Copy link
Member

Any update on this? I need it for a project so happy to do the final tweaks if you don't have time.

@theduke
Copy link
Member Author

theduke commented Jun 18, 2019

I removed the requirement for the transparent attribute.

I would'nt support anything other than single field tuple structs for now though. Better to be conservative and reconsider once someone asks for it.

So should be good to merge.

@mwilliammyers
Copy link

Just to nitpick - should we take out #[graphql(transparent)] from the examples in the docs?

theduke and others added 3 commits June 25, 2019 17:59
This commit implements a newtype style custom derive
for scalars via `#[derive(GraphQLScalarValue)]`, which now
supports both deriving a base enum scalar type and newtypes.

For newtypes, the `#[graphql(transparent)]` attribute is
required.

This commit:
* implements the derive
* adds integration tests
* updates the book
@theduke theduke force-pushed the scalar-derive-transparent branch from 6ca3806 to 0268d03 Compare June 25, 2019 16:00
@theduke theduke merged commit 97e1005 into graphql-rust:master Jun 25, 2019
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.

ScalarValue custom derive: newtype support
4 participants