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

Allow copying a ParameterizedTypeName with new type arguments #1140

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

ansman
Copy link
Contributor

@ansman ansman commented Aug 31, 2021

This fixes #1139

Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

Thanks!

@ansman ansman force-pushed the feature/copy-type-arguments branch from 095dcbd to d582179 Compare August 31, 2021 20:16
@ZacSweers
Copy link
Collaborator

From an API standpoint I wonder if it would be better to have a withParameters instead. These overloaded copy functions are brittle historically

@ansman
Copy link
Contributor Author

ansman commented Sep 1, 2021

I'd be OK with that too. In fact I was probably going to add a mapTypeArguments in my project too.

@Egorand
Copy link
Collaborator

Egorand commented Sep 1, 2021

IMO copy is easier to discover with it being a Kotlin pattern. What's causing brittleness, the fact that it's hard to change in a backwards-compatible way?

@ansman
Copy link
Contributor Author

ansman commented Sep 2, 2021

If we are keeping copy I think it might make sense to expose the enclosing type and add both that and the raw type to the copy method.

@Egorand
Copy link
Collaborator

Egorand commented Sep 10, 2021

@ansman I'd be reluctant to expose those simply for consistency, without a use case. If there is a use case then we'll consider it.

@ansman
Copy link
Contributor Author

ansman commented Sep 10, 2021

My current use cases are:

  • Creating a wildcarded typename.
  • Mapping Java types to Kotlin types (List, Set, Annotation etc).

This isn't possible in all cases without exposing these things.

@ZacSweers
Copy link
Collaborator

Can you add a test case that wasn't possible before? The one here is possible with the previous API

@ansman ansman force-pushed the feature/copy-type-arguments branch from d582179 to f053ffb Compare September 20, 2021 13:47
@ansman ansman force-pushed the feature/copy-type-arguments branch from f053ffb to ba18571 Compare September 20, 2021 13:51
@ansman
Copy link
Contributor Author

ansman commented Sep 20, 2021

Done

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

This seems ok to me, though that example has me 🤔🤔🤔

@ansman
Copy link
Contributor Author

ansman commented Sep 20, 2021

Yeah, the example isn't perhaps real since you'd access this as Map.Entry<...>. I essentially needed a nested class and the enclosing class needs to be parameterized to test the specific case.

@ZacSweers ZacSweers merged commit 6d2f2e4 into square:master Sep 20, 2021
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.

Support copying a ParameterizedTypeName with different type arguments
3 participants