-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
lint: add a linter to prohibit map[...]bool in favor of map[...]struct{} #85689
Conversation
Curious what people think about such a linter in general @cockroachdb/sql-queries. |
I'm generally in favor. As you say there are cases where the trinary logic of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @yuzefovich)
The linter is currently limited to `sql/opt/norm` package. This commit was prompted by an article mentioned in the Golang Weekly newsletter. The rationale is that `map[...]struct{}` is more efficient, but in some cases the bool map value is actually desired, so this commit introduces an opt-out `//nolint:maptobool` comment. Release justification: low-risk performance improvement. Release note: None
Add an opt-out to disable the linter. TFTRs! bors r+ |
Build failed (retrying...): |
Build succeeded: |
The linter is currently limited to
sql/opt/norm
package. This commitwas prompted by an article mentioned in the Golang Weekly newsletter.
The rationale is that
map[...]struct{}
is more efficient, but in somecases the bool map value is actually desired, so this commit introduces
an opt-out
//nolint:maptobool
comment.Release justification: low-risk performance improvement.
Release note: None