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 TypeReference.createInstance(Class<T>) #13600

Conversation

alzimmermsft
Copy link
Member

This PR adds the method <T> TypeReference<T> createInstance(Class<T>) to TypeReference. It allows for the creation and caching of non-parameterized TypeReferences.

@alzimmermsft alzimmermsft added Client This issue points to a problem in the data-plane of the library. Azure.Core azure-core labels Jul 29, 2020
@alzimmermsft alzimmermsft self-assigned this Jul 29, 2020
* @return Either the cached or new instance of {@link TypeReference}.
*/
@SuppressWarnings("unchecked")
public static <T> TypeReference<T> createInstance(Class<T> clazz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks a little weird by having both createInstance and public constructor.

Copy link
Member Author

Choose a reason for hiding this comment

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

They serve different purposes. The public constructor is for anonymous instances that use parameterized types for their T, such as Map<String, String>. This factory method is meant for non-parameterized Class types that can be safely cached and maintain T.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is a bit odd. They serve different purposes but the constructor works for all cases and if the caller always uses the ctor not knowing about the static createInstance method, it may defeat the purpose of caching.
Also, in azure-core, there is a TypeUtil that has a utility method to createParameterizedType() that may be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

The public constructor work with most of the cases like (concrete class, paramterized type), but not the clazz variable cases.
E.g.

public void someMethod(Class<T> clazz) {
    TypeReference<T> type = new TypeReference<T>() {} // type is null in this case, no matter what clazz you passed in.
    ...
 }

In this case, we must use the createInstance API. However, createInstance API does not serve all purpose as well, we cannot pass the parameterized type here.

So both APIs have hard limitations and caching strategy are different behind the API. If I am not expert users I would confuse of the usage here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed @sima-zhu, having both does lead to some confusion. Luckily, this type is mainly going to be used internally where determining whether to use createInstance(Class<T>) or the constructor will be based on the API. If the type is known ahead of time and it is parameterized the only choice is to use the constructor, if the type is known but is a Class<T> or is passed into the API createInstance(Class<T>) will be used.

@alzimmermsft alzimmermsft merged commit 796236b into Azure:master Jul 30, 2020
@alzimmermsft alzimmermsft deleted the AzExperimental_AddCreateInstanceToTypeReference branch July 30, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants