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

Add our own @Deprecated annotation + directive #1736

Closed
jmartisk opened this issue Feb 10, 2023 · 5 comments · Fixed by #1738
Closed

Add our own @Deprecated annotation + directive #1736

jmartisk opened this issue Feb 10, 2023 · 5 comments · Fixed by #1738
Assignees
Labels
good first issue Good for newcomers

Comments

@jmartisk
Copy link
Member

Add a @io.smallrye.graphql.api.Deprecated annotation that has an optional String reason parameter and is transformed into the schema as a directive.
The directive should look the same as described in https://spec.graphql.org/draft/#sec--deprecated
This can be a task for @mskacelik

@jmartisk jmartisk added the good first issue Good for newcomers label Feb 10, 2023
@michaelbird1155
Copy link

michaelbird1155 commented Feb 10, 2023

It would be cool to extend or implement java.lang.Deprecated as well so that IDEs see the field as deprecated. However, I understand the flexibility of having both options can be benefitial. On the other hand, if someone wants both do we really want the below?

@Deprecated
@java.lang.Deprecated
private String myField

Perhaps we give it a slightly different name like @Gql_Deprecated?

@t1
Copy link
Collaborator

t1 commented Feb 10, 2023

I'd also rather reuse the existing java.lang.Deprecated or reading the code becomes messy. They both have the same semantics, so I think it would be okay. And in case you really need it, a custom @Deprecated annotation would be trivial to add.

@jmartisk
Copy link
Member Author

java.lang.Deprecated has two params that we don't want (since and forRemoval) and doesn't have the one param that we do want (reason). Where would we read the reason from?

@jmartisk
Copy link
Member Author

My main point is we want to have an annotation with an API that exactly matches the API of the directive mentioned in the GraphQL spec.
Maybe we could go with both - transform java.lang.Deprecated into a @deprecated directive without a reason,
but also have our own annotation that has a reason parameter, which would be added to the directive.

@t1
Copy link
Collaborator

t1 commented Feb 13, 2023

Fair point. It's just ugly to have several annotations on your classpath triggering different behaviour. Supporting both would be the best compromise, I guess.

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

Successfully merging a pull request may close this issue.

4 participants