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

feat: replace gomnd by mnd #4572

Merged
merged 2 commits into from
Mar 28, 2024
Merged

feat: replace gomnd by mnd #4572

merged 2 commits into from
Mar 28, 2024

Conversation

ldez
Copy link
Member

@ldez ldez commented Mar 24, 2024

Rename gomnd to mnd:

  • Deprecate gomnd.
  • Add mnd.

This PR also fixes the problem of consistency between the analyzer name and linter name like #4567.

Before
$ golangci-lint run 
main.go:28:23: mnd: Magic number: 200, in <condition> detected (gomnd)
        if res.StatusCode != 200 {
                             ^
main.go:21:12: mnd: Magic number: 2, in <assign> detected (gomnd)
                Timeout: 2 * time.Second,
                         ^
With the PR
$ golangci-lint run 
main.go:28:23: Magic number: 200, in <condition> detected (mnd)
        if res.StatusCode != 200 {
                             ^
main.go:21:12: Magic number: 2, in <assign> detected (mnd)
                Timeout: 2 * time.Second,
                         ^

@ldez ldez added enhancement New feature or improvement linter: update Update the linter implementation inside golangci-lint labels Mar 24, 2024
@ldez ldez requested a review from bombsimon March 24, 2024 17:55
@ldez ldez mentioned this pull request Mar 24, 2024
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

LGTM, as long as the name actually is just mnd. I see it's set to mnd in the code but most of the references refer to it as go-mnd and so does the gopher image in the repo.

@tommy-muehle Can you confirm that the name of the linter should be just mnd or would you rather set the name to gomnd on the analyzer? I guess mnd makes more sense since it's the name of the binary as well but might as well ask to clarify.

@ldez
Copy link
Member Author

ldez commented Mar 24, 2024

I see it's set to mnd in the code but most of the references refer to it as go-mnd and so does the gopher image in the repo.

So you are saying that only my first commit (coming from the previous PR) was enough, it's because you need more PRs to review? 😸

@ldez ldez added the blocked Need's direct action from maintainer label Mar 24, 2024
@bombsimon
Copy link
Member

So you are saying that only my first commit (coming from the previous PR) was enough, it's because you need more PRs to review? 😸

I'm sorry if this yielded extra work for you. I was mostly asking a question and didn't have the time to reply to your comments in the other PR before you created this one. I thought, and I think you did as well based on this PR, that the name is actually mnd and with that in mind I think this PR is the right thing to do. However I see no downside in asking the author about the name.

If the linter would be named gomnd I think an upstream PR would make more sense than the overwriting in the other PR anyway.

I approved this PR because maybe you have different context or feel different about this so feel free to merge. I did not try to get more PRs to review.

@ldez
Copy link
Member Author

ldez commented Mar 24, 2024

I'm sorry if this yielded extra work for you.

I was just kidding 😸

I was torn between the name change and the deprecation, so I used your comment as justification.

@ldez
Copy link
Member Author

ldez commented Mar 24, 2024

More seriously, the readme is a mix of go-mnd and mnd but the command, the binary, the analyzer, and the brew-tap are called mnd.
Only the Docker image and the repository are named go-mnd.

We will wait for the feedback of the author but mnd seems to be the right name.

@bombsimon
Copy link
Member

bombsimon commented Mar 24, 2024

We will wait for the feedback of the author but mnd seems to be the right name.

Yeah 100% agree! I pinged to see if we got some response within a day or two max but I wouldn't block too long on it.

pkg/lint/lintersdb/builder_linter.go Show resolved Hide resolved
pkg/golinters/mnd.go Show resolved Hide resolved
@ldez ldez added this to the next milestone Mar 28, 2024
@ldez ldez removed the blocked Need's direct action from maintainer label Mar 28, 2024
@ldez ldez merged commit 288c847 into golangci:master Mar 28, 2024
13 checks passed
@ldez ldez deleted the feat/mnd branch March 28, 2024 20:23
@ldez ldez modified the milestones: next, v1.58 May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement linter: update Update the linter implementation inside golangci-lint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants