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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gosimple linter to .travis.yml #457

Merged
merged 1 commit into from Apr 25, 2017

Conversation

Projects
None yet
3 participants
@y0ssar1an
Copy link
Contributor

y0ssar1an commented Apr 22, 2017

gosimple is a neat little code linter from the author of staticcheck. It catches code quality issues like the following:

// should be if boolVar
if boolVar == true {}

// should be time.Since(x)               
time.Now().Sub(x)

// should be return err                        
if err != nil {
    return err
}
return nil 

The full list of checks is available here.

It only found one issue, which is very good for a project this size. Kudos to all the contributors! 馃挴

@googlebot googlebot added the cla: yes label Apr 22, 2017

for _, e := range external {
root, err := sm.DeduceProjectRoot(e)
if err != nil {
errs = append(errs, string(root))
continue
}

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 23, 2017

Contributor

staticcheck found this errs slice which is never used. At first I was going to return an error if len(errs) > 0. Then I looked at the DeduceProjectRoot code and realized that whenever err != nil, root == "". In the current code, if DeduceProjectRoot returns an error, errs will be a slice of empty strings.

I "fixed" the issue by deleting errs entirely. Should we do more to handle the error here?

This comment has been minimized.

@sdboyer

sdboyer Apr 24, 2017

Member

@y0ssar1an ah sorry, I noticed this over in another PR and pushed a temporary fix directly. (This is actually a real problem, but fixing it requires going back to the drawing board a bit on how status works, as it entails a new output mode)

This comment has been minimized.

@sdboyer

sdboyer Apr 24, 2017

Member

So, yeah, roll back this one part, and we can merge this in 馃憤

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 24, 2017

Contributor

Done! Glad to hear you already got this. 馃槃 馃憤

@@ -369,12 +369,10 @@ func runStatusAll(out outputter, p *dep.Project, sm *gps.SourceMgr) error {
rm, _ := ptree.ToReachMap(true, true, false, nil)

external := rm.Flatten(false)
roots := make(map[gps.ProjectRoot][]string)
var errs []string
roots := make(map[gps.ProjectRoot][]string, len(external))

This comment has been minimized.

@y0ssar1an

y0ssar1an Apr 23, 2017

Contributor

We know the maximum required capacity for roots is the number of external imports, so we should specify the capacity here.

@y0ssar1an y0ssar1an changed the title Add gosimple linter to .travis.yml Add gosimple linter to .travis.yml. Fix new staticcheck issue. Apr 24, 2017

Ryan Boehning
Add gosimple linter to .travis.yml
Fix one trivial gosimple issue in test/integration_testproj.go. The prog
variable was being declared on one line, then assigned to on the next
line. I combined the two lines into one and now gosimple is happy.

Fix ineffective append caught by new check added to staticcheck on April
21st, 2017. A string slice was being appended to, but the slice was
never used. The fix was to delete the slice altogether.
@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 25, 2017

LGTM - thanks!

@sdboyer sdboyer merged commit e025682 into golang:master Apr 25, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@y0ssar1an y0ssar1an changed the title Add gosimple linter to .travis.yml. Fix new staticcheck issue. Add gosimple linter to .travis.yml Apr 25, 2017

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#457 from y0ssar1an/master
Add gosimple linter to .travis.yml. Fix new staticcheck issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment