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.

Sign up for GitHub

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

Jump to bottom

Fixing megacheck issues reported by gometalinter #236

Merged
merged 1 commit into from Jun 11, 2018
Merged

Fixing megacheck issues reported by gometalinter #236

merged 1 commit into from Jun 11, 2018

Conversation

tariq1890
Copy link
Contributor

Fixes the following issues reported :

check/utils.go:375:25:warning: HasSuffix is a pure function but its return value is ignored (SA4017) (megacheck)
download/download.go:86:3:warning: this value of err is never used (SA4006) (megacheck)
handlers/about.go:14:2:warning: redundant return statement (S1023) (megacheck)
handlers/badge.go:52:10:warning: the argument is already a string, there's no need to use fmt.Sprintf (S1025) (megacheck)
handlers/check.go:54:2:warning: redundant return statement (S1023) (megacheck)
handlers/check.go:100:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (megacheck)
handlers/check.go:159:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (megacheck)
tools/clean-repos/main.go:29:8:warning: should use time.Since instead of time.Now().Sub (S1012) (megacheck)
tools/db/manage_db.go:60:3:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (megacheck)
tools/db/manage_db.go:60:3:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (megacheck)
tools/db/manage_db.go:113:20:warning: should omit comparison to bool constant, can be simplified to !*listDupes (S1002) (megacheck)
tools/names/migrate_repo_names.go:79:3:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (megacheck)
tools/scores/init_repo_scores.go:15:2:warning: const metaBucket is unused (U1000) (megacheck)

@tariq1890
Copy link
Contributor Author

Changes are all refactors and none to the existing logic.

shawnps
shawnps reviewed Dec 28, 2017
View reviewed changes
download/download.go Outdated
@@ -83,7 +83,7 @@ func download(path, dest string, firstAttempt bool) (root *vcs.RepoRoot, err err
if err != nil && firstAttempt {
// may have been rebased; we delete the directory, then try one more time:
log.Printf("Failed to update %q (%v), trying again...", root.Repo, err.Error())
err = os.RemoveAll(fullLocalPath)
os.RemoveAll(fullLocalPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's log the err here rather than remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Can do that

tools/scores/init_repo_scores.go Outdated
@@ -11,8 +11,7 @@ import (
)

const (
repoBucket string = "repos"
metaBucket string = "meta"
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to metaBucket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an unused variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see now, thanks

@tariq1890
Copy link
Contributor Author

@shawnps I've made the required changes as per your review comments.

@tariq1890
Copy link
Contributor Author

tariq1890 commented Feb 9, 2018 •
edited

Hey @shawnps @hermanschaaf . Can this be merged?

Also, I would like to discuss this issue #235. Considering the popularity of goreportcard, adding megacheck would have significant positive impact in my opinion.

@tariq1890
Copy link
Contributor Author

Following up on this.

shawnps
shawnps reviewed Jun 11, 2018
View reviewed changes
download/download.go Outdated
@@ -84,6 +84,9 @@ func download(path, dest string, firstAttempt bool) (root *vcs.RepoRoot, err err
// may have been rebased; we delete the directory, then try one more time:
log.Printf("Failed to update %q (%v), trying again...", root.Repo, err.Error())
err = os.RemoveAll(fullLocalPath)
if err != nil {
log.Printf("Failed to delete directory : %s", fullLocalPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick but could you remove the space before the : here?

log.Printf("Failed to delete directory: %s", fullLocalPath)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@shawnps
Copy link
Contributor

shawnps commented Jun 11, 2018

LGTM

@shawnps shawnps merged commit 6ecdf3c into gojp:master Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shawnps shawnps shawnps left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tariq1890 @shawnps