Skip to content

Infer graphql "deprecation" from #[deprecated(note = "...")] in derive (and macros) #269

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 11 commits into from
Oct 28, 2018

Conversation

kestred
Copy link
Contributor

@kestred kestred commented Oct 23, 2018

Follow on to #194.

Derive

Updates the GraphQLObject and GraphQLEnum derives to use the first #[deprecated] annotation with a note = ... to supply the deprecation text, which can still be overridden with #[graphql(deprecation = ...)] like in #195.

Macros

Additionally, this PR adds support for #[doc = ...] and #[deprecated(note = ...)] to the graphql_object! and graphql_interface! macros, which has some benefits:

  • Lets your field name(args...) line-up so that they're easier to scan (even with a deprecation)`

  • Allows mutliple doc strings be combined akin to the collapse_docs behavior.
    (including any const SOME_DESC: &'static str if you want w/ #[doc = SOME_DESC])

  • Makes it easier to refactor a #[derive(GraphQLObject)] into a graphql_object!, e.g. a contrived example

    #[derive(GraphQLObject)]
    struct Plus {
         left: i32,
         right: i32,
    
         #[deprecated(note = "This has been deprecated because it doesn't handle overflow")]
         result: i32,
     }
    
    // after converting to use macro
    graphql_object!(Plus: () |&self| {
         field left() -> i32 { self.left }
         field right() -> i32 { self.right }
    
         #[deprecated(note = "This has been deprecated because it doesn't handle overflow")]
         field result() -> i32 { self.result }
         field checked_sum() -> Option<i32> { self.left.checked_sum(self.right) }
    })

@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #269 into master will increase coverage by 0.05%.
The diff coverage is 79.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #269      +/-   ##
==========================================
+ Coverage   88.99%   89.05%   +0.05%     
==========================================
  Files         102      102              
  Lines       20678    21039     +361     
==========================================
+ Hits        18403    18736     +333     
- Misses       2275     2303      +28
Impacted Files Coverage Δ
juniper/src/macros/common.rs 100% <ø> (ø) ⬆️
juniper_codegen/src/derive_enum.rs 0% <0%> (ø) ⬆️
juniper_codegen/src/derive_object.rs 0% <0%> (ø) ⬆️
juniper_tests/src/codegen/derive_input_object.rs 66.01% <0%> (ø) ⬆️
juniper/src/schema/schema.rs 84.74% <100%> (+0.56%) ⬆️
juniper/src/macros/object.rs 98.59% <100%> (ø) ⬆️
juniper/src/macros/interface.rs 95.23% <100%> (+0.43%) ⬆️
juniper/src/executor/mod.rs 93.8% <100%> (ø) ⬆️
juniper_codegen/src/util.rs 77.51% <40%> (+3.4%) ⬆️
juniper/src/schema/meta.rs 86.1% <80.88%> (+1.09%) ⬆️
... and 12 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 cf7e8df...1262be3. Read the comment docs.

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! 🍻 It looks great ❤️ . One thing I would like to see supported is the bare #[deprecated] form, as one could reasonably expect it to work and I don't believe the GraphQL spec says a reason is required (https://facebook.github.io/graphql/June2018/#sec-Deprecation). I did similar stuff in graphql-client if you want to take a look.

Also, please add an entry to the changelog.

@kestred
Copy link
Contributor Author

kestred commented Oct 24, 2018

Oh; I didn't realize the reason was optional; I'll definitely update to support the bare deprecation

@kestred
Copy link
Contributor Author

kestred commented Oct 25, 2018

@LegNeato: Changelog, docs, etc added--- build failure on Azure is only due to spurious network error.

Can you have it retry?

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes so quickly! Would it be too much trouble to split the doc parsing into a separate PR? It's hard to review with both the deprecation changes and doc changes in one PR.

@@ -90,13 +93,24 @@ impl EnumVariantAttrs {
continue;
}
if let Some(AttributeValue::String(val)) =
keyed_item_value(&item, "deprecated", AttributeValidation::String)
keyed_item_value(&item, "deprecation", AttributeValidation::String)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deprecated, right? And if so, it looks like repeated code and can be removed, as the next block is doing match keyed_item_value(&item, "deprecated", AttributeValidation::String

Copy link
Contributor Author

@kestred kestred Oct 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially yes.

This check appears to be unnecessary for backwards compatibility, however I believe the reason that the line is in the EnumVariantAttrs impl is because I was using the same code block to handle it for both that and ObjectFieldAttrs.

In the current version (specifically on master), the attribute name is inconsistent between enum variants which use deprecated and object fields which use deprecation.

This change, makes both support the same set of attributes in a backwards-compatible way. Happy to remove it if that is undesirable.

If the use of deprecation on object fields is itself considered deprecated, I can remove this one on EnumAttrs, and if desired can also remove the same one on ObjFieldAttrs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah. Sorry, forgot about that previous support.

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.

3 participants