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
Refactor: rewrite Merge
method to address readability and efficiency
#97794
Conversation
@chendave: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I don't think this PR is increasing readability. However, I do think this function is hard to read. I would reconsider if you can come up with an alternate rewrite of the function that is more readable. |
right, this doesn't move the readability needle. |
64d911a
to
e5f0395
Compare
e5f0395
to
64d911a
Compare
/hold |
64d911a
to
5bda740
Compare
/hold cancel |
@alculquicondor How about this version? |
5bda740
to
e21e2ea
Compare
/retest |
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.
Don't forget to update title and description
1e29006
to
71c1179
Compare
/retitle "Refactor: rewrite |
Merge
method to address readability and efficiency"
/retitle Refactor: rewrite Merge method to address readability and efficiency |
Merge
method to address readability and efficiency"Merge
method to address readability and efficiency
71c1179
to
dc4b4f0
Compare
/retest |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, chendave The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Dave Chen <dave.chen@arm.com>
dc4b4f0
to
3c197c5
Compare
/lgtm |
Signed-off-by: Dave Chen dave.chen@arm.com
@ahg-g @lixiang233 I came across this line of code today, and I do think this line of code is a legacy nit, the status code has been set in the below snippet of code, but in the loop it was set for each plugin, consider the case that status map hold the status for each of plugin, this kind of instruction is not effective and unnecessary.
kubernetes/pkg/scheduler/framework/interface.go
Lines 202 to 208 in c5cc25d
For the sake of other status code might be added in the future, we can tweak the code a little bit.
@ahg-g what do you think?
What type of PR is this?
Add one of the following kinds:
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: