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

Enable nilerr linter & fix findings #12270

Merged
merged 14 commits into from
Apr 18, 2023
Merged

Enable nilerr linter & fix findings #12270

merged 14 commits into from
Apr 18, 2023

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Apr 13, 2023

Enable the nilerr linter in golangci-lint and fix a few of its findings, which I believe are minor but valid.

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

There are four calls to ValidatorAtIndex that will return false, nil instead of false, err. Like this:

val, err := st.ValidatorAtIndex(valIdx)
if err != nil {
return false, nil
}

I'm not sure if this is error state is even reachable, but just in case it is, I think we should return err.

In other places, this is handled as expected. Like this:

proposer, err := beaconState.ValidatorAtIndex(proposerIndex)
if err != nil {
return err
}

But if returning nil there is intended, I think it's worthy of a comment.


Edit: this also fixes some findings from the CI lint check (I didn't see these locally).

For one's that seemed fine, I added: //nolint:nilerr

@jtraglia jtraglia requested a review from a team as a code owner April 13, 2023 20:45
terencechain
terencechain previously approved these changes Apr 13, 2023
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@jtraglia
Copy link
Contributor Author

Hmm the CI has more findings of nilerr. I didn't see those locally. Will check them out.

terencechain
terencechain previously approved these changes Apr 13, 2023
terencechain
terencechain previously approved these changes Apr 13, 2023
jtraglia and others added 2 commits April 17, 2023 10:33
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
terencechain
terencechain previously approved these changes Apr 17, 2023
@prestonvanloon
Copy link
Member

This is failing some unit tests. Possibly due to poor test setup.

rkapka
rkapka previously approved these changes Apr 18, 2023
@prylabs-bulldozer prylabs-bulldozer bot merged commit b6181f8 into prysmaticlabs:develop Apr 18, 2023
@jtraglia jtraglia deleted the fix-nilerr branch April 18, 2023 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants