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

DefaultExcludePatterns should only be used for specified linter #1494

Merged
merged 1 commit into from Nov 12, 2020

Conversation

zhangyunhao116
Copy link
Member

@zhangyunhao116 zhangyunhao116 commented Nov 6, 2020

Fixes #1474

This PR add "EXC0011", because most of package in golangci-lint does not having a package comment. make test passed.


updated, for now, "EXC0011" same with "EXC0002" but with different linter field for keeping backward compatible.

@CLAassistant
Copy link

CLAassistant commented Nov 6, 2020

CLA assistant check
All committers have signed the CLA.

@zhangyunhao116
Copy link
Member Author

zhangyunhao116 commented Nov 6, 2020

passed all tests, ready for review.

@zhangyunhao116 zhangyunhao116 force-pushed the fix-default-exclude2 branch 2 times, most recently from a4ddfda to 5b51dc8 Compare November 6, 2020 07:57
@iwankgb
Copy link
Contributor

iwankgb commented Nov 7, 2020

I think that what you are doing here (changing semantics of an existing flag) is not a good idea. I would rather expect new flag to be introduced and handle the new case. Adding // nolint:stylecheck in quite a few places seems to be a prove that this change is not backward compatible and in the same time it does not seem to be a bug fix.

@zhangyunhao116
Copy link
Member Author

The // nolint:stylecheck is added because // nolint:dupl is not a valid package comment, and makes the test failed(even add a package comment in package golinters). This problem will be fixed if dominikh/go-tools#871 is merged, then we can remove these // nolint:stylecheck, and add a valid package comment to package golinters.

@zhangyunhao116
Copy link
Member Author

@iwankgb we can keep backward compatible by adding a new rule like

	{
		ID: "EXC0012",
		Pattern: "(comment on exported (method|function|type|const)|" +
			"should have( a package)? comment|comment should be of the form)",
		Linter: "stylecheck",
		Why:    "Annoying issue about not having a comment. The rare codebase has such comments",
	},

Then we can remove all these // nolint:stylecheck. But i don't know which is better, maybe someone from the team can give some ideas @golangci-releaser

@zhangyunhao116
Copy link
Member Author

zhangyunhao116 commented Nov 8, 2020

After put some thought to it, i think keeping backward compatible makes this PR more easily to understand, and we can remove all //nolint:stylecheck, but it cover some mistakes in testdata and stylecheck.

@iwankgb
Copy link
Contributor

iwankgb commented Nov 8, 2020

@ZYunH now I understand what you mean. I believe that your PR and issue are basically a bugfix and bugreport. If we provide linter information then we should not apply default exclude rules to all the linters. Taking above into consideration I have only one request: provide tests for GetExcludePatterns() and exclude processor use-case.
First test should be pretty straightforward. In case of the second one you will have to dig into testdata directory and prepare a file with matching violations that would fail on master and pass on your branch. Let me know if you have any issues, I will try to help :)

@zhangyunhao116
Copy link
Member Author

@iwankgb PTAL, thanks!

Copy link
Contributor

@iwankgb iwankgb left a comment

Choose a reason for hiding this comment

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

This is last final piece of change and I will be more than happy to merge the PR.

pkg/config/config_test.go Outdated Show resolved Hide resolved
@iwankgb iwankgb merged commit 9948153 into golangci:master Nov 12, 2020
@golangci-automator
Copy link

Hey, @ZYunH — we just merged your PR to golangci-lint! 🔥🚀

golangci-lint is built by awesome people like you. Let us say “thanks”: we just invited you to join the GolangCI organization on GitHub.
This will add you to our team of maintainers. Accept the invite by visiting this link.

By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.
More information about contributing is here.

Thanks again!

@ldez ldez added this to the v1.33 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefaultExcludePatterns should only be used for specified linter
4 participants