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

fix #751 [rule.add-constant] add ignoreFuncs to exclude constants in … #756

Merged
merged 5 commits into from
Oct 13, 2022

Conversation

rawen17
Copy link
Contributor

@rawen17 rawen17 commented Oct 7, 2022

Closes #751

Copy link
Collaborator

@chavacava chavacava left a comment

Choose a reason for hiding this comment

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

I've left some comments
Please fill the PR description by using the PR description template

In general I think regexps are too powerful (or complicated?) to be used without the risk of ignoring things you don't know you are ignoring. For example, the regexp .Println used in the tests will not ignore just "methods named Println of any package" but also any method or function named such as Println is a suffix of the name (myPrintln, ThisIsAnotherPrintln, dPrintlln, ...)

RULES_DESCRIPTIONS.md Outdated Show resolved Hide resolved
rule/add-constant.go Outdated Show resolved Hide resolved
rule/add-constant.go Show resolved Hide resolved
@xobotyi
Copy link

xobotyi commented Oct 11, 2022

The problem is out of thin air🙃

We face the regexp if not every day, but commonly and there is nothing "overcomplicated" with it, it is just a right tool for such situation, most of existing linters included in golangcilint uses regexp.

Example you've given easily solvable by reading the docs and acknowledging that it is a regexp and then heading to regexp101. And easily avoidable by gicing some defauls and\or examples in docs.

Anyways having regexes is much better and easier option that needing to exclude separately all 36 argument methods having defaults from flag package, and same for pflag.

@chavacava
Copy link
Collaborator

My point is that RE are very error prone (at the point that your own test case .Println was wrong)
I'm not against the use of RE and I agree that providing clear doc will help in avoiding subtle errors; for example in the rule documentation it should be interesting to highlight that fmt.Println in the given example, will make revive ignore fmt.Println, fmtXPrintln, fmt1Println, an so on.

@rawen17
Copy link
Contributor Author

rawen17 commented Oct 13, 2022

It was my fault, i had to write regexp os\\.*,fmt\\.Println,make in documentation, but i wrote os.*,fmt.Println,make
I will fix it

@chavacava chavacava merged commit 3d83403 into mgechev:master Oct 13, 2022
@chavacava
Copy link
Collaborator

It was my fault, i had to write regexp os\\.*,fmt\\.Println,make in documentation, but i wrote os.*,fmt.Println,make I will fix it

@rawen17 thanks for the PR, I've already merged it.
Sorry @xobotyi I did not notice that I was answering to you (and not to the author of the PR)

@rawen17
Copy link
Contributor Author

rawen17 commented Oct 13, 2022

@chavacava Ok, thanks, i will create new PR for this changes

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.

[rule.add-constant] add ''ignoreFuncs' to exclude constants in functions
3 participants