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

[291] Enable lint errcheck #501

Merged
2 commits merged into from
Nov 23, 2022
Merged

[291] Enable lint errcheck #501

2 commits merged into from
Nov 23, 2022

Conversation

pbains
Copy link
Contributor

@pbains pbains commented Nov 16, 2022

Scope

  1. Enabling linting on save for file in VS Code for errcheck.
  2. Resolve errcheck issues currently on master.

Why?

Code maintenance and readability

Todos

Keep the issue open because more linting needs to be completed before closing.

@github-actions github-actions bot added the go Pull requests that update Go code label Nov 16, 2022
@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Base: 53.90% // Head: 54.36% // Increases project coverage by +0.45% 🎉

Coverage data is based on head (483ab98) compared to base (1a4312e).
Patch coverage: 33.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #501      +/-   ##
==========================================
+ Coverage   53.90%   54.36%   +0.45%     
==========================================
  Files         345      369      +24     
  Lines       42685    42708      +23     
  Branches      599      599              
==========================================
+ Hits        23011    23218     +207     
+ Misses      17239    17028     -211     
- Partials     2435     2462      +27     
Impacted Files Coverage Δ
consensus/objs/rs.go 60.65% <ø> (ø)
layer1/executor/task_executor.go 90.57% <25.00%> (-0.91%) ⬇️
layer1/executor/task_manager.go 72.72% <36.36%> (+3.05%) ⬆️
bridge/contracts/Foundation.sol 37.50% <0.00%> (-12.50%) ⬇️
badgerTrie/smt_merkle_proof.go 65.62% <0.00%> (-3.91%) ⬇️
bridge/contracts/AliceNetFactory.sol 89.74% <0.00%> (-0.74%) ⬇️
bridge/contracts/ValidatorPool.sol 88.71% <0.00%> (-0.34%) ⬇️
bridge/contracts/ETHDKG.sol 84.42% <0.00%> (-0.13%) ⬇️
bridge/contracts/Snapshots.sol 93.80% <0.00%> (-0.06%) ⬇️
bridge/contracts/BToken.sol 94.87% <0.00%> (-0.05%) ⬇️
... and 75 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Congrats on your first PR! Whooo!

.golangci.yaml Show resolved Hide resolved
cmd/testutils/layer1_tx_spammer/main.go Outdated Show resolved Hide resolved
cmd/testutils/layer1_tx_spammer/main.go Outdated Show resolved Hide resolved
consensus/objs/rs.go Outdated Show resolved Hide resolved
@@ -78,7 +81,7 @@ func TestGetSetNode(t *testing.T) {
db := initializeDB()

node := &Node{}
db.rawDB.Update(func(txn *badger.Txn) error {
_ = db.rawDB.Update(func(txn *badger.Txn) error {
Copy link

Choose a reason for hiding this comment

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

Should check and t.Fatal on this.

@@ -124,7 +127,7 @@ func TestGetSetLinkedList(t *testing.T) {
db := initializeDB()

ll := &LinkedList{}
db.rawDB.Update(func(txn *badger.Txn) error {
_ = db.rawDB.Update(func(txn *badger.Txn) error {
Copy link

Choose a reason for hiding this comment

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

Should check and t.Fatal on this.

@@ -135,7 +136,7 @@ type ManagerResponseInfo struct {
type requestStored struct {
BaseRequest
WrappedTask *marshaller.InstanceWrapper `json:"wrappedTask"`
killedAt uint64 `json:"killedAt"`
killedAt uint64 `json:"-"`
Copy link

Choose a reason for hiding this comment

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

Should pull this out of the PR unless it's needed by another change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@@ -121,7 +122,10 @@ func (tm *TaskManager) eventLoop() {

case taskRequest, ok := <-tm.requestChan:
if !ok {
tm.onError(ErrReceivedRequestClosedChan)
err := tm.onError(ErrReceivedRequestClosedChan)
Copy link

Choose a reason for hiding this comment

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

tm.onError always returns an error, no need to check if it's not nil. Probably can just _ this error.

@pbains pbains changed the title [291] Enable errcheck [291] Enable lint errcheck Nov 16, 2022
@pbains pbains marked this pull request as ready for review November 23, 2022 14:59
@pbains pbains requested a review from a team as a code owner November 23, 2022 14:59
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM!

@ghost ghost merged commit 38e4427 into main Nov 23, 2022
@ghost ghost deleted the pbains/291-lint-errcheck branch November 23, 2022 16:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant