-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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 New function #70973
Refactor New function #70973
Conversation
@ping035627: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
Hi @ping035627. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/assign @timothysc |
/unassign @timothysc |
pkg/scheduler/scheduler.go
Outdated
@@ -231,6 +209,40 @@ func New(client clientset.Interface, | |||
return sched, nil | |||
} | |||
|
|||
func InitPolicyFromFile(policyFile string, policy *schedulerapi.Policy) error { |
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.
Do we need this to be exported? It was not previously so there are apparently no users for this (yet).
If it's exported it should have a comment documenting it.
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.
I think we should export it for clean code and reduce its complexity.
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.
I don't see how exporting it reduces the code's complexity, but if it's exported it should have a documentation string in the form:
// InitPolicyFromFile ...
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.
Thanks for your advice, it should be added annotation actually.
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.
+1 to what @miguelbernadi said. These functions shouldn't be exported.
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.
I just think about the point of clean code. Function should be small and do one thing, so I think we can extract the method for clean code and reduce its complexity.
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.
Sorry it seems we miscommunicated. I understand your point on extracting the code into a function. My concern is about exporting it outside the package (Upper-case methods are public in the package, therefore exported).
It would be fine just to make the methods package private, starting with lowercase, like:
func initPolicyFromFile(...
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.
oh, misunderstand really, I have modified, thanks for your advice.
pkg/scheduler/scheduler.go
Outdated
return nil | ||
} | ||
|
||
func InitPolicyFromConfigMap(client clientset.Interface, policyRef *kubeschedulerconfig.SchedulerPolicyConfigMapSource, policy *schedulerapi.Policy) error { |
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.
same as above.
47dee4d
to
61d181f
Compare
/release-note-none |
61d181f
to
9e76073
Compare
Signed-off-by: PingWang <wang.ping5@zte.com.cn> add comments for InitPolicyFromFile Signed-off-by: PingWang <wang.ping5@zte.com.cn> make the methods package private Signed-off-by: PingWang <wang.ping5@zte.com.cn>
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.
/lgtm
/approve
Thanks, @ping035627!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, ping035627 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 |
/ok-to-test |
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Refactor New function to reduce its complexity and improve readability.
Which issue(s) this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
NONE