Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add TypeReference.createInstance(Class<T>) #13600
Changes from all commits
ece7eea
535b8dd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It looks a little weird by having both createInstance and public constructor.
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.
They serve different purposes. The public constructor is for anonymous instances that use parameterized types for their
T
, such asMap<String, String>
. This factory method is meant for non-parameterizedClass
types that can be safely cached and maintainT
.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.
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.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.
The public constructor work with most of the cases like (concrete class, paramterized type), but not the clazz variable cases.
E.g.
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.
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.
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 aClass<T>
or is passed into the APIcreateInstance(Class<T>)
will be used.