-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
[FLINK-37441] Rename factoryHierarchy
to typeHierarchy
for TypeExtractor
#26278
base: master
Are you sure you want to change the base?
Conversation
52b3287
to
ce2414a
Compare
factoryHierarchy
to typeHierarchy
for TypeExtractor
|
||
final List<Type> factoryHierarchy = new ArrayList<>(typeHierarchy); | ||
final TypeInfoFactory<? super OUT> factory = getClosestFactory(factoryHierarchy, t); | ||
final List<Type> newTypeHierarchy = new ArrayList<>(typeHierarchy); |
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.
nit: it is not a hierarchy it is a list - if we are changing this why not change to something more representative like candidateTypeList
and rename the related variables to be Lists not hierarchy.
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 typeHierarchy
also is a list, so I followed the style.
But I think you said is right, let's use typeHierarchyList
and newTypeHierarchyList
.
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.
After a second thought. I guess the original author use the name typeHierarchy
due to the list contains the type information according to the hierarchy.
Such as:
{class java.lang.String, class java.lang.Object}.
So let me temporarily use the name typeHierarchy
. I am willing to use typeHierarchyList
and newTypeHierarchyList
if more reviewer suggest it.
@flinkbot run azure |
cc @1996fanrui |
What is the purpose of the change
This PR proposes to rename
factoryHierarchy
totypeHierarchy
due to it is just aList<Type>
and doesn't related to factory.Brief change log
Rename
factoryHierarchy
totypeHierarchy
forTypeExtractor
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation