Goimports #4881

tac0turtle · 2019-08-09T18:33:54Z

added goimports
changed golangci.yml to only show what we dont have enabled

Signed-off-by: Marko Baricevic marbar3778@yahoo.com

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [-t <tag>] [-m <msg>]
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

- added goimports - changed golangci.yml to only show what we dont have enabled Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

fedekunze · 2019-08-09T18:37:04Z

.golangci.yml

+    - maligned
+    - nakedret
+    - ineffassign
+    - errcheck


this should be enabled

lots of errors, some stuff is steaming from tests, i can disable this, or would we want the errrors on tests as well?

same, blocked on the testing refactors

.golangci.yml

fedekunze · 2019-08-09T18:37:43Z

.golangci.yml

    - govet
-    - unused
-    - deadcode
+    - golint


this should be enabled

it currently has about 20+ errors, can spend time next week enabling it

will enable this after @colin-axner 's testing refactors have been merged

.golangci.yml

codecov · 2019-08-09T19:01:02Z

Codecov Report

Merging #4881 into master will increase coverage by <.01%.
The diff coverage is 49.49%.

@@            Coverage Diff             @@
##           master    #4881      +/-   ##
==========================================
+ Coverage   53.74%   53.74%   +<.01%     
==========================================
  Files         272      272              
  Lines       17105    17103       -2     
==========================================
- Hits         9193     9192       -1     
- Misses       7221     7224       +3     
+ Partials      691      687       -4

x/supply/internal/types/supply.go

x/gov/keeper/test_common.go

x/gov/test_common.go

alexanderbez

ACK

baseapp/baseapp.go

client/keys/add.go

x/simulation/transition_matrix.go

x/staking/keeper/delegation_test.go

x/distribution/keeper/delegation_test.go

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

alexanderbez · 2019-08-16T16:08:10Z

@marbar3778 please signal when this is r4r again.

tac0turtle · 2019-08-16T18:13:04Z

@alexanderbez its ready, just knocked another linter out quickly

fedekunze

LGTM

alexanderbez

ACK still stands. We should also enabled unparam

rigelrozanski

Good cleanup - I have several suggestions, but also there are some tests which should have not been deleted which should be reinstated (this last comment is blocking for me)

x/supply/internal/types/supply.go

x/staking/keeper/validator_test.go

rigelrozanski · 2019-08-16T19:35:16Z

x/staking/keeper/validator_test.go

@@ -292,146 +292,6 @@ func TestValidatorBasics(t *testing.T) {
 	require.False(t, found)
 }

-// test how the validators are sorted, tests GetBondedValidatorsByPower
-func GetValidatorSortingUnmixed(t *testing.T) {


Why was this test deleted? These are vital tests of functionality which must be kept unless this is clearly duplicated somewhere (blocking comment)

i got this as unused code and couldn't find its uses used in the codebase, do we want to keep unused code around? I'm fine with putting it back

We definitely want both these tests which have been deleted in this file

just for the heads up these tests fail and arent currently run, will put them back in and comment them out

@marko, let's not comment them out, but rather add t.SkipNow() at the very top of the test function. Also, let's open up an issue and reference it to be fixed asap.

i.e.

func GetValidatorSortingUnmixed(t *testing.T) { // TODO: Evaluate and fix broken unit test. // ref: link t.SkipNow() // ... }

I second @alexanderbez

added it in

x/staking/keeper/validator_test.go

rigelrozanski · 2019-08-16T19:41:40Z

simapp/state.go

@@ -135,7 +135,7 @@ func AppStateFromGenesisFileFn(r *rand.Rand, config simulation.Config) (json.Raw
 		r.Read(privkeySeed)

 		privKey := secp256k1.GenPrivKeySecp256k1(privkeySeed)
-		newAccs = append(newAccs, simulation.Account{privKey, privKey.PubKey(), acc.Address})
+		newAccs = append(newAccs, simulation.Account{PrivKey: privKey, PubKey: privKey.PubKey(), Address: acc.Address})


Create and use a new function simulation.NewAccount(......

This isn’t necessary (as in it shouldn’t be blocking) but can be added if the constructor is concise.

rigelrozanski · 2019-08-16T19:43:52Z

store/types/utils.go

@@ -82,9 +82,8 @@ func PrefixEndBytes(prefix []byte) []byte {

 // InclusiveEndBytes returns the []byte that would end a
 // range query such that the input would be included
-func InclusiveEndBytes(inclusiveBytes []byte) (exclusiveBytes []byte) {
-	exclusiveBytes = append(inclusiveBytes, byte(0x00))
-	return exclusiveBytes


I would recommend reverting this change, even though it's more verbose, I find the variable names helpful in understanding this functionality

would it be better to better the comment instead of reverting?

I think the variables would be clearer than a comment. But this is minor for me

x/gov/keeper/querier_test.go

rigelrozanski · 2019-08-16T19:48:35Z

x/staking/genesis.go

@@ -156,7 +156,7 @@ func ExportGenesis(ctx sdk.Context, keeper Keeper) types.GenesisState {
 	})
 	var lastValidatorPowers []types.LastValidatorPower
 	keeper.IterateLastValidatorPowers(ctx, func(addr sdk.ValAddress, power int64) (stop bool) {
-		lastValidatorPowers = append(lastValidatorPowers, types.LastValidatorPower{addr, power})
+		lastValidatorPowers = append(lastValidatorPowers, types.LastValidatorPower{Address: addr, Power: power})


Create and use a new function types.NewLastValidatorPower(.......

…-sdk into marko/addgoimports

alexanderbez · 2019-08-19T13:25:33Z

x/staking/keeper/validator_test.go

@@ -292,146 +292,6 @@ func TestValidatorBasics(t *testing.T) {
 	require.False(t, found)
 }

-// test how the validators are sorted, tests GetBondedValidatorsByPower
-func GetValidatorSortingUnmixed(t *testing.T) {


@marko, let's not comment them out, but rather add t.SkipNow() at the very top of the test function. Also, let's open up an issue and reference it to be fixed asap.

i.e.

func GetValidatorSortingUnmixed(t *testing.T) { // TODO: Evaluate and fix broken unit test. // ref: link t.SkipNow() // ... }

x/staking/keeper/validator_test.go

alexanderbez

ACK; we should open up an issue for those two failing tests though.

rigelrozanski

ACK - provided (@alexanderbez new comment is addressed) #4881 (comment)

👏 🌟 🌵

tac0turtle · 2019-08-19T15:15:14Z

#4925

Goimports

8eb1634

- added goimports - changed golangci.yml to only show what we dont have enabled Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

tac0turtle requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners August 9, 2019 18:33

fedekunze suggested changes Aug 9, 2019

View reviewed changes

alessio reviewed Aug 10, 2019

View reviewed changes

x/supply/internal/types/supply.go Show resolved Hide resolved

alessio reviewed Aug 10, 2019

View reviewed changes

x/supply/internal/types/supply.go Show resolved Hide resolved

alexanderbez added the R4R label Aug 15, 2019

alexanderbez suggested changes Aug 15, 2019

View reviewed changes

x/gov/keeper/test_common.go Outdated Show resolved Hide resolved

x/gov/test_common.go Outdated Show resolved Hide resolved

x/gov/test_common.go Outdated Show resolved Hide resolved

tac0turtle added 3 commits August 15, 2019 16:33

Merge branch 'master' into marko/addgoimports

7dcc4c9

address pr comments

cbab6b0

gofmt & sylecheck

263ee5d

alexanderbez approved these changes Aug 15, 2019

View reviewed changes

baseapp/baseapp.go Show resolved Hide resolved

alexanderbez requested a review from fedekunze August 15, 2019 14:58

alexanderbez added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. next-point-release labels Aug 15, 2019

tac0turtle added 3 commits August 15, 2019 17:24

more linting

228bfeb

more linter

8bc531d

Merge branch 'master' into marko/addgoimports

5499d91

fedekunze reviewed Aug 15, 2019

View reviewed changes

tac0turtle and others added 4 commits August 15, 2019 18:11

Apply suggestions from code review

5fca4b5

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

fix lint

c8d97f6

gocritic add

3163973

minor cleanup

093d856

alexanderbez requested a review from fedekunze August 16, 2019 19:16

fedekunze approved these changes Aug 16, 2019

View reviewed changes

alexanderbez approved these changes Aug 16, 2019

View reviewed changes

alexanderbez removed the next-point-release label Aug 16, 2019

Merge branch 'master' into marko/addgoimports

d5b421b

rigelrozanski suggested changes Aug 16, 2019

View reviewed changes

tac0turtle added 2 commits August 18, 2019 23:01

add deleted tests

ee8e8c7

Merge branch 'marko/addgoimports' of https://github.com/cosmos/cosmos…

8bb929f

…-sdk into marko/addgoimports

alexanderbez suggested changes Aug 19, 2019

View reviewed changes

t.SkipNow()

57f761f

alexanderbez approved these changes Aug 19, 2019

View reviewed changes

alexanderbez requested a review from rigelrozanski August 19, 2019 14:30

rigelrozanski approved these changes Aug 19, 2019

View reviewed changes

Merge branch 'master' into marko/addgoimports

771827b

tac0turtle mentioned this pull request Aug 19, 2019

Validator tests #4925

Closed

alexanderbez merged commit 3a4f1fc into master Aug 19, 2019

alexanderbez deleted the marko/addgoimports branch August 19, 2019 16:06

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#4881: Linting Galore

e1ae31b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Goimports #4881

Goimports #4881

tac0turtle commented Aug 9, 2019 •

edited

Loading

fedekunze Aug 9, 2019

tac0turtle Aug 9, 2019 •

edited

Loading

fedekunze Aug 15, 2019

fedekunze Aug 9, 2019

tac0turtle Aug 9, 2019

fedekunze Aug 15, 2019

codecov bot commented Aug 9, 2019 •

edited

Loading

alexanderbez left a comment

alexanderbez commented Aug 16, 2019

tac0turtle commented Aug 16, 2019

fedekunze left a comment

alexanderbez left a comment

rigelrozanski left a comment

rigelrozanski Aug 16, 2019

tac0turtle Aug 16, 2019

rigelrozanski Aug 18, 2019

tac0turtle Aug 18, 2019 •

edited

Loading

alexanderbez Aug 19, 2019 •

edited

Loading

rigelrozanski Aug 19, 2019

tac0turtle Aug 19, 2019

rigelrozanski Aug 16, 2019

alexanderbez Aug 16, 2019 •

edited

Loading

rigelrozanski Aug 16, 2019

tac0turtle Aug 16, 2019 •

edited

Loading

rigelrozanski Aug 18, 2019

rigelrozanski Aug 16, 2019

alexanderbez Aug 16, 2019

alexanderbez Aug 19, 2019 •

edited

Loading

alexanderbez left a comment

rigelrozanski left a comment

tac0turtle commented Aug 19, 2019

Goimports #4881

Goimports #4881

Conversation

tac0turtle commented Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

tac0turtle Aug 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2019 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez commented Aug 16, 2019

tac0turtle commented Aug 16, 2019

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle Aug 18, 2019 • edited Loading

Choose a reason for hiding this comment

alexanderbez Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Aug 19, 2019 • edited Loading

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

tac0turtle commented Aug 19, 2019

tac0turtle commented Aug 9, 2019 •

edited

Loading

tac0turtle Aug 9, 2019 •

edited

Loading

codecov bot commented Aug 9, 2019 •

edited

Loading

tac0turtle Aug 18, 2019 •

edited

Loading

alexanderbez Aug 19, 2019 •

edited

Loading

alexanderbez Aug 16, 2019 •

edited

Loading

tac0turtle Aug 16, 2019 •

edited

Loading

alexanderbez Aug 19, 2019 •

edited

Loading