Removed redundant if and return statements #129730
Conversation
|
|
|
Welcome @adriancuadrado! |
|
Hi @adriancuadrado. 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adriancuadrado The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 779a75444340611d60d1907bf9e55cae88da7dcb
|
|
/retest |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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-sigs/prow repository. |
|
/reopen |
|
@adriancuadrado: Reopened this PR. In response to this:
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-sigs/prow repository. |
|
/remove-lifecycle rotten |
| @@ -756,10 +756,7 @@ func (r *Request) watchInternal(ctx context.Context) (watch.Interface, runtime.D | |||
| isErrRetryableFunc := func(request *http.Request, err error) bool { | |||
| // The watch stream mechanism handles many common partial data errors, so closed | |||
| // connections can be retried in many cases. | |||
| if net.IsProbableEOF(err) || net.IsTimeout(err) { | |||
There was a problem hiding this comment.
we had several discussions about this in the past, and there is no performance benefit since the code will be compiled anyway, and the majority of us find that being explicit is easier to read and understand or that just is not meaningful to pollute the git history for a change like this
I suggest better find other places for contribute that need more help than changing this noop places
There was a problem hiding this comment.
I'll leave this PR open just in case someone says something different but if the consensus is that it's better the way you say then feel free to close this PR. I personally don't agree but I don't want to get in the way of the decisions of your team.
There was a problem hiding this comment.
way of the decisions of your team.
there are mixing feelings depending on the people, not really a team, up to you to leave it open, I personally do not see much difference, just that with one I do not need to do the mental exercise, but is not a big thing
I personally don't agree
can you be more specific on the disagreement?
There was a problem hiding this comment.
the majority of us find that being explicit is easier to read and understand
I agree in general but not in this case.
In my humble opinion, this kind of construct looks more like someone didn't pay attention to what they were programming and didn't realize they could just return the boolean value directly:
if(something) {
return true;
} else {
return false;
}I don't think adding redundant if statements like this one makes the code much more readable. If anything, it makes some people like me wonder why wouldn't you just return the value directly.
But really, these kind of things are ultimately personal opinions and I don't want to waste too much of anyone's time over something this insignificant. I just assumed the original author didn't pay attention so I made this tiny PR to fix that. Didn't expect to have any kind of discussion over the coding style to be honest, and didn't really want to bother anyone too much. Sorry about that.
There was a problem hiding this comment.
no need to say sorry, I find this conversation interesting, thanks for the feedback
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Improves the quality of the source code
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
N/A
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
N/A