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

Provide a way to create a copy of a Key with an annotation #1108

Closed
wants to merge 1 commit into from
Closed

Provide a way to create a copy of a Key with an annotation #1108

wants to merge 1 commit into from

Conversation

kashike
Copy link
Contributor

@kashike kashike commented Jun 2, 2017

This allows you to easily specify an annotation/annotation type to an existing key.

@sameb
Copy link
Member

sameb commented Jun 2, 2017

Don't really think this is necessary. Trivial to do already:
Key.get(key.getTypeLiteral(), annotation)

@kashike
Copy link
Contributor Author

kashike commented Jun 2, 2017

It was to provide the opposite of Key#ofType which makes sense to have as it deals with the AnnotationStrategy itself.

There's not a huge difference with changing the annotation, one just involves less duplication:
key.withAnnotation(annotation)
Key.get(key.getTypeLiteral(), annotation)

If you don't think this is necessary, that's fine - feel free to close.

@sameb
Copy link
Member

sameb commented Jun 2, 2017

ofType is slightly more necessary, as construction a new variant with a different type requires some branching (to deal with annotation instances vs annotation types).

@sameb sameb closed this Jun 2, 2017
@dimo414
Copy link
Contributor

dimo414 commented Jun 2, 2017

Personally, the discoverability here is at least a bit of a selling point. More than once I've looked for an ofType() sibling for annotations in Key and realized I need to instead use the factory method.

@sameb
Copy link
Member

sameb commented Jun 2, 2017

if you think it's worth it extra methods, feel free to merge in.

@dimo414 dimo414 reopened this Jun 2, 2017
@kashike
Copy link
Contributor Author

kashike commented Apr 12, 2018

Any chance of this being pulled soon, @dimo414?

@kashike
Copy link
Contributor Author

kashike commented Sep 20, 2018

Any chance of getting this in soon? 😺

@kashike
Copy link
Contributor Author

kashike commented May 21, 2020

Is there a chance of this getting in soon?

@dimo414
Copy link
Contributor

dimo414 commented May 21, 2020

I'll take a swing at it over the long weekend :)

@kashike
Copy link
Contributor Author

kashike commented May 21, 2020

Thank you! 😄

@kevinb9n kevinb9n mentioned this pull request May 26, 2020
kevinb9n pushed a commit that referenced this pull request May 26, 2020
…nt annotations.

Closes #1108

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=313246519
@dimo414
Copy link
Contributor

dimo414 commented May 27, 2020

Thanks for the contribution, and your patience :)

@dimo414 dimo414 closed this May 27, 2020
kevinb9n pushed a commit that referenced this pull request May 27, 2020
…nt annotations.

Closes #1108

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=313246519
ShoOgino pushed a commit to ShoOgino/guiceFile that referenced this pull request Oct 14, 2020
…nt annotations.

Closes google/guice#1108

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=313246519
ShoOgino pushed a commit to ShoOgino/guiceMethod that referenced this pull request Oct 14, 2020
…nt annotations.

Closes google/guice#1108

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=313246519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants