-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add boolset linter #6096
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
base: main
Are you sure you want to change the base?
feat: add boolset linter #6096
Conversation
Signed-off-by: Artur Melanchyk <[email protected]>
Hey, thank you for opening your first Pull Request ! |
In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.
Pull Request Description
Base RequirementsThese requirements are not declarative; the team will verify them.
Linter
The Linter Tests Inside Golangci-lint
|
I think that IMHO, this is an "early optimization" linter: there is no problem of using For now, I don't know if this linter will be accepted; I need to think about it. |
Hi @ldez, I appreciate your feedback.
In Golang there isn't a built-in workDone := make(map[string]bool)
for _, item := range workItems {
// can be both true and false
workDone[item] = DoSomeWork()
}
for _, item := range workItems {
if workDone[item] {
// value successfully processed earlier
} else {
// calculation failed but we do not wat to recalculate
}
} However this is quite rare case and my linter will not complain about such use cases. The suggested linter warns |
I think the I agree this is a small optimization in most cases, but I also think a lot of developers aren't aware of ZST (one could say this is as close as we get in Go) and simply avoid this pattern because they're unaware of it. This is something I use personally even for small collections just to make it clear it's a set and not a map I want to represent, a complement to good variable naming without having to put the type in the variable name. So even though the readme mentions memory and performance, I think this might be just as valid as a code style linter. I do realize a lot of users wants to roll their own implementation and avoid dependencies, I wonder if encouraging something like deckarep/golang-set to even better represent a set would be a good/better idea. |
as an example, here is IMHO linter should just highlight the issue and not suggest using specific implementations |
This is an over-interpretation of your PRs:
I have a problem evaluating this linter as a "style" linter because, like prealloc, it will be used as a performance requirement instead of something that should be evaluated on a case-by-case basis. I have already closed several PRs on my various projects related to preallocating slices or maps in unnecessary cases or on other types of "early optimizations". I still need to think about this linter.
I agree that a linter should not recommend a lib, and this lib specifically should be avoided because of the dependency on the mongo-driver. |
This linter is at the borderline between a detector and a linter. Based on my analysis of several projects, this linter has false negatives (ex: I don't want to reproduce the No decision for now. |
The PR adds support for boolset linter
boolset
is a Go linter that findsmap[T]bool
values which are effectively used as sets and recommends switching tomap[T]struct{}
.The replacement avoids storing redundant boolean payloads and can cut memory usage for large maps in half while keeping semantics identical.