-
Notifications
You must be signed in to change notification settings - Fork 428
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
Graphql object proc macro #333
Graphql object proc macro #333
Conversation
@ForsakenHarmony thanks for starting this, and sorry it took so long for us to get the ball rolling. |
8adea08
to
ad8c8ea
Compare
77febb5
to
651007e
Compare
Codecov Report
@@ Coverage Diff @@
## master #333 +/- ##
==========================================
- Coverage 88.32% 86.36% -1.97%
==========================================
Files 103 105 +2
Lines 14922 15348 +426
==========================================
+ Hits 13180 13255 +75
- Misses 1742 2093 +351
Continue to review full report at Codecov.
|
@theduke Will this work with interfaces? It is currently not very ergonomic to have to implement a trait and use I might need to open a new issue but if using this with interfaces will not work then would it be possible to use a trait + |
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.
Looks pretty good. Does this increase the minimum rust version?
- Formatting is failing
- No Changelog entry
@mwilliammyers interfaces are another topic, but we can definitely make these more convenient. We can discuss in #295 . |
f1b42fd
to
e045ee4
Compare
Alright! This was a lot of work, but it's ready now.
A couple of notes: Rust versionSadly the Rust version must be bumped to DiagnosticsError messages could be improved quite a bit in some cases with RFC 1566, but that doesn't seem close to stabilization. Deprecation
Argument customizationArgument customization is pretty awkward right now. For now we'll have to live with: #[graphql(
arguments(
arg1(
description = "..",
default = true
)
)
)]
fn field(arg1: bool) -> bool { arg1 } Re-exportsI've fixed up the re-exports in juniper/src/lib.rs Also, I've started to manually re-export all items so rustdoc shows documentation for them. |
d798d38
to
a3eebda
Compare
@LegNeato ping |
a3eebda
to
738a44e
Compare
This commit implements a new proc macro `impl_object` that replaces the old graphql_object! macro. The code shares a lot of similarities with the GraphQLObject custom derive, so the code was unified to handle both more generically. Also, doc comment processing was standardized and improved.
This commit deprecates the graphql_object macro and replaces all of it's uses with the new impl_object proc macro. (Except for the old macro tests). This commit also adds new integration tests for impl_object.
Remove the internal macros from re-export. This was a mistake. Also, import each item from juniper_codegen manually to enable rustdoc integration.
d7a45f6
to
e5226de
Compare
e5226de
to
de12e0e
Compare
Needed for releasing automation.
0ac8eaf
to
552b4d0
Compare
It uses a regex using |
Patch juniper_codegen dev dependency on juniper.
Alright, found it. Needed to add the replacement config in |
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.
Arguments look pretty ugly in this new world :-/. Also not really in love with impl_object
name, but can't think of anything better right now.
Yeah as mentioned above, arguments can become nicer once the accepted RFC for attributes on function parameters is implemented. That will enable fn field(
&self,
#[graphql(default = true, description = "arg")]
arg: bool,
) -> &str {
}
Me neither. I initially stuck with |
a2b2cc3
to
29025e6
Compare
Alright, let's do this. PS: @LegNeato , please check gitter! |
This picks up form @ForsakenHarmony s work.
I squashed the commits down to one, but preserved the contribution.
Old pr: #181
( tried to push to @ForsakenHarmony s branch, but the force push apparently closed the old PR and I can't reopen it)
My commit finishes up the implementation.
Todo:
NOTE: I decided to go for a different syntax than previously discussed.
Main motivation:
Check the
integration_tests/src/codegen/impl_object.rs
file for the tests.It looks like this: