Skip to content
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

remove unnecessary else block on test #16897

Closed
moficodes opened this issue Nov 9, 2023 · 8 comments · Fixed by #16889 or #16903
Closed

remove unnecessary else block on test #16897

moficodes opened this issue Nov 9, 2023 · 8 comments · Fixed by #16889 or #16903

Comments

@moficodes
Copy link
Member

What would you like to be added?

in etcdserver/api/v3discovery/discovery_test.go:266:9

else {
fkv.t.Errorf("unexpected key: %s", key)
return nil, fmt.Errorf("unexpected key: %s", key)
}

We can remove the else block and return right away.

Why is this needed?

make verify fails with indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (revive).

@jmhbnz
Copy link
Member

jmhbnz commented Nov 9, 2023

Hey @moficodes - Thanks for raising this, which golangci-lint version are you using?

The project currently uses 1.53.3 which does not flag this issue, refer https://github.com/etcd-io/etcd/blob/main/.github/workflows/static-analysis.yaml#L18

I note pull request #16889 is related to this.

If we proceed with fixing this which I'm not opposed to, ideally we would also bump golangci-lint to a version which identifies this issue and fix any other instances detected (if any).

@serathius
Copy link
Member

If we proceed with fixing this which I'm not opposed to, ideally we would also bump golangci-lint to a version which identifies this issue and fix any other instances detected (if any).

I think we kept golangci-lint old to avoid allowing go1.21 features. I think we should revisit this decision.

@moficodes
Copy link
Member Author

This failed on 1.55.2.

This is the only linting error that comes up.

@moficodes
Copy link
Member Author

If we want to keep it at that version, I think we need to update Contributing docs to make sure developers have the correct environment setup.

@jmhbnz
Copy link
Member

jmhbnz commented Nov 9, 2023

I think we kept golangci-lint old to avoid allowing go1.21 features. I think we should revisit this decision.

Based on the discussion in #16661 (comment) the 1.21 discussion was in relation to the use of new builtins rather than updating golangci-lint specifically?

I agree we should try and update the project version and if there is a blocker update CONTIBUTING.md if we can't.

@jmhbnz
Copy link
Member

jmhbnz commented Nov 9, 2023

With #16889 merged latest golangci-lint version 1.55.2 runs successfully for me.

@moficodes would you be able to raise a pr to propose adopting this version?

@moficodes
Copy link
Member Author

On it! Should I create a new Issue? or this is good?

@jmhbnz
Copy link
Member

jmhbnz commented Nov 9, 2023

We can do it under this issue ☺

moficodes added a commit to moficodes/etcd that referenced this issue Nov 9, 2023
fixes: etcd-io#16897
update golangci-lint version to the latest to keep the codebase linting up to date.

Signed-off-by: Mofi Rahman <mofi@google.com>
moficodes added a commit to moficodes/etcd that referenced this issue Nov 9, 2023
fixes: etcd-io#16897
update golangci-lint version to the latest to keep the codebase linting up to date

Signed-off-by: Mofi Rahman <mofi@google.com>
etsrpl pushed a commit to e-tienne/etcd that referenced this issue Nov 15, 2023
fixes: etcd-io#16897
update golangci-lint version to the latest to keep the codebase linting up to date

Signed-off-by: Mofi Rahman <mofi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants