-
Notifications
You must be signed in to change notification settings - Fork 828
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
Complete deprecated fields and arguments support #1472
Complete deprecated fields and arguments support #1472
Conversation
Codecov ReportBase: 95.97% // Head: 95.98% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #1472 +/- ##
=======================================
Coverage 95.97% 95.98%
=======================================
Files 51 51
Lines 1739 1742 +3
=======================================
+ Hits 1669 1672 +3
Misses 70 70
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
That was fast! Thank you for the PR :) I'll check it out ASAP! |
@erikwrede I felt some courage to play with |
@erikwrede Hey! Have you had a chance to take a look? |
@vhutov Sorry for taking to long, I was actually just looking at your new changes right now. If you just need the deprecation for now, maybe it would probably best to split up the PRs and have a more fundamental discussion about refactoring the meta classes in the other PR |
@erikwrede no worries, I'm not in rush. I just thought it'd be better to add Meta support as well. I can certainly split them in two, if you think it's better. Thanks for looking at this! |
Having thought about it again, think it's better to split up the two PRs and think about the If we are talking about a redesign of that part, I'd personally prefer to explicitly get the inheritable fields from a type, without having to implement a second A rough concept could look something like this: Assume we have a base class (roughly based on the actual class InputObjectTypeOptions:
description: str
deprecation_reason: str
abcd: str we would have an explicit definition of inheritable fields from that class. It is also extendable by subclassing it. We could now adapt this for mounted types like InputObjectType.meta_type = InputObjectTypeOptions
class ArgumentOptions:
description: str
deprecation_reason: str
required: bool
Mount InputObjectType
# Extract deprecation reason from InputObjectTypeOptions (InputObjectType._meta)
# Don't extract required: field not defined on InputObjectTypeOptions
# Don't extract abcd: field not defined on ArgumentOptions If no This is just an initial idea, and by no means a finalized design. Also open to suggestions on why we should use |
f6fc761
to
f28e3e5
Compare
@erikwrede cleaned up the PR to contain only the deprecation reason ✅ |
UPD: Changed PR description |
f28e3e5
to
e9c1676
Compare
@erikwrede Could you take a look? |
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.
Review for this was still on my list - thank you for updating the PR!
I noticed that you made changes to field.py
to only allow deprecation on nullable fields. According to the Spec, this constraint only applies to input fields.
Look at the following example from spec:
type ExampleType {
invalidField(
newArg: String
oldArg: String! @deprecated(reason: "Use `newArg`.")
): String
}
With input fields, the restriction makes sense, as we don't have any alternative to passing a value to the required oldField
, although oldField
is deprecated. For input fields, the alternative is to not query the field anymore.
The rest looks good, thank you!
graphene/types/field.py
Outdated
assert ( | ||
deprecation_reason is None | ||
), f"Field {name} is required, cannot deprecate it." |
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.
This restriction only applies to input object fields & arguments : https://spec.graphql.org/draft/#sec--deprecated
description="Create a user", | ||
deprecation_reason="Is deprecated", | ||
required=True, | ||
required=False, | ||
) | ||
|
||
field = MyMutation._meta.fields["create_user"] | ||
assert field.name == "createUser" | ||
assert field.description == "Create a user" | ||
assert field.deprecation_reason == "Is deprecated" | ||
assert field.type == NonNull(CreateUser) | ||
assert field.type == CreateUser | ||
|
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.
See field.py
e9c1676
to
de78461
Compare
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.
LGTM!
As a follow-up, we should rework the get_introspection_query()
for schema to allow for optional args to be introspected. Currently schema.introspect()
calls this using the default args that set input_value_deprecation: bool = False,
, excluding any deprecated fields or args.
Will release tonight in |
Thanks! |
Implementing enhancement from #1446
In this PR I ensure that
Argument
,Field
andInputField
support deprecation functionality.TODOs from the issue:
Skipped this one, as meta is not populated to the Mounted object.
Notes:
Field
andInputField
already had this field, whereasArgument
didn't.InputField
andArgument
.Enum
andField
were compliant.InputObject
orEnum
implicitly, meta fields are not populated to theMounted
object.