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

Added go-mnd linter #842

Merged
merged 12 commits into from
Nov 16, 2019
Merged

Added go-mnd linter #842

merged 12 commits into from
Nov 16, 2019

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Nov 5, 2019 •
edited
Loading

Fixes #441

Please make sure you didn't directly change README.md: it should be changed only by changing README.tmpl.md and running make README.md.

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

golangcibot
golangcibot reviewed Nov 5, 2019
View reviewed changes
golangcibot
golangcibot reviewed Nov 5, 2019
View reviewed changes
golangcibot
golangcibot reviewed Nov 5, 2019
View reviewed changes
@sayboras
Copy link
Member Author

sayboras commented Nov 5, 2019

Build failed due to un-related reason, other PR failed for the same reason.

@sayboras sayboras marked this pull request as ready for review November 5, 2019 12:17
@tpounds
Copy link
Contributor

tpounds commented Nov 5, 2019

@sayboras The build is failing due to not regenerating the README.md file after adding your changes. Try running make README.md to see if it generates any changes and add them to a commit.

@tpounds tpounds added the linter: new Support new linter label Nov 5, 2019
@sayboras
Copy link
Member Author

sayboras commented Nov 5, 2019

@tpounds it's failing even after updating Readme.md :(

@tpounds
Copy link
Contributor

tpounds commented Nov 5, 2019

Looks like it's failing for an unrelated issue due to a change with the --deadline flag.

https://travis-ci.com/golangci/golangci-lint/jobs/253051370#L1258

I think it was introduced in by commit df27992.

@jirfag Any thoughts?

@sayboras
Copy link
Member Author

sayboras commented Nov 7, 2019

@tpounds @jirfag I just fixed the unit test in #845

@sayboras
Copy link
Member Author

sayboras commented Nov 7, 2019

@tpounds the build is passed now, need your help to review

tpounds
tpounds reviewed Nov 8, 2019
View reviewed changes
@tpounds
Copy link
Contributor

tpounds commented Nov 8, 2019

@sayboras Overall, this looks good. Can you add a few test cases to ensure this linter works as expected? I've linked a few prs below that have examples of how to do this.

#771
#787

tpounds
tpounds reviewed Nov 8, 2019
View reviewed changes
go.mod
@@ -37,6 +37,7 @@ require (
github.com/spf13/viper v1.4.0
github.com/stretchr/testify v1.4.0
github.com/timakin/bodyclose v0.0.0-20190930140734-f7f2e9bca95e
github.com/tommy-muehle/go-mnd v0.0.0-20190903201840-c93e405da530
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to reference a tagged release version?

Copy link
Member Author

@sayboras sayboras Nov 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I tried before, but it gives me the below error (even the tag is available)

go: github.com/tommy-muehle/go-mnd@v1.1.1: reading github.com/tommy-muehle/go-mnd/go.mod at revision v1.1.1: unknown revision v1.1.1

Copy link
Member Author

Choose a reason for hiding this comment

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

oh I got it now, the tag in github.com/tommy-muehle/go-mnd is not prefixed with v, so we can only use pseudo-versions only

Copy link
Member Author

Choose a reason for hiding this comment

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

raised one issue here tommy-muehle/go-mnd#1

jirfag
jirfag requested changes Nov 9, 2019
View reviewed changes
Copy link
Contributor

@jirfag jirfag left a comment

Choose a reason for hiding this comment

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

thank you,
please fix load mode and I'm ready to merge it

sayboras and others added 9 commits November 12, 2019 08:49
@sayboras
Initial commit
2a08573
@sayboras
Fixed goimports
3577294
@sayboras @golangcibot
Update pkg/golinters/mnd.go
d3e5aa4 
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
@sayboras
Run goimports
dbb8ec3
@sayboras @golangcibot
Update pkg/golinters/mnd.go
40a19ea 
Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com>
@sayboras
Add prefix for goimport local
c7c2e19
@sayboras
Run make README.md
061c111
@sayboras
Incorporate review comments
073501b 
Add test cases
@sayboras
Update readme
64cc91c
@sayboras
Copy link
Member Author

@jirfag @tpounds @bombsimon Addressed all comments, please let me know if i miss anything

@bombsimon
Copy link
Member

I'm not a collaborator but since you asked and since I commented your PR, lgtm. 🚢

@sayboras
Copy link
Member Author

I'm not a collaborator but since you asked and since I commented your PR, lgtm.

Thanks for your time reviewing anyway 💯

tpounds
tpounds reviewed Nov 16, 2019
View reviewed changes
pkg/golinters/gomnd.go
package golinters

import (
magic_numbers "github.com/tommy-muehle/go-mnd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would just alias this as mnd.

tpounds
tpounds reviewed Nov 16, 2019
View reviewed changes
pkg/golinters/gomnd.go
"github.com/golangci/golangci-lint/pkg/golinters/goanalysis"
)

func NewGomnd() *goanalysis.Linter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Similarly, I would name this NewGoMND to be consistent with standard Go naming conventions.

tpounds
tpounds approved these changes Nov 16, 2019
View reviewed changes
Copy link
Contributor

@tpounds tpounds left a comment

Choose a reason for hiding this comment

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

@sayboras 🚢 it. Feel free to address nitpicks in a separate pr.

@tpounds tpounds merged commit bd29216 into golangci:master Nov 16, 2019
@sayboras sayboras deleted the feature/go-mnd branch November 16, 2019 03:14
@sayboras sayboras mentioned this pull request Nov 16, 2019
Merged
@sayboras
Copy link
Member Author

@tpounds thanks for your review, I have addressed above comments in #859

@sayboras sayboras mentioned this pull request Nov 28, 2019
Merged
@ldez ldez added this to the v1.22 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bombsimon bombsimon bombsimon left review comments

@tpounds tpounds tpounds approved these changes

@jirfag jirfag Awaiting requested review from jirfag

@golangcibot golangcibot Awaiting requested review from golangcibot

Assignees
No one assigned
Labels
linter: new Support new linter
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

Add support for mnd (Magic Number Detector)
7 participants
@sayboras @CLAassistant @tpounds @bombsimon @jirfag @golangcibot @ldez