-
Notifications
You must be signed in to change notification settings - Fork 9.8k
refactor: use slices.Contains to simplify code #16523
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
Conversation
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.
Thanks for this, and welcome to the Prometheus project!
What you did is absolutely fine, but it made me think some code could now be simplified a bit further. Some ideas below.
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.
lgtm, thanks!
let's have another pair of eyes look at this
Thanks for your review. Modified. Please review again. |
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.
Pull Request Overview
This PR refactors several code sections to replace manual loops with the standard library's slices.Contains, simplifying and clarifying membership checks.
- Simplifies the code in postings, tests, labels, Kubernetes discovery, config, and main packages by replacing explicit loops with slices.Contains.
- Improves overall readability without altering behavior.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tsdb/index/postings.go | Replaced loop with slices.Contains to check for EmptyPostings(). |
rules/manager_test.go | Simplified metric name check using slices.Contains. |
model/labels/regexp.go | Replaced explicit loops with slices.Contains in matchers. |
model/labels/labels_common.go | Used slices.Contains instead of loop for label filtering. |
discovery/kubernetes/kubernetes.go | Simplified role selector validation with slices.Contains. |
config/config.go | Reordered imports and replaced loop with slices.Contains in API version validation. |
cmd/prometheus/main.go | Refactored duplicate flag value check using slices.Contains. |
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.
LGTM, but you'll need to fix linting.
Modified. Please review again. |
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.
Looking great! One small thing in imports.
Signed-off-by: hardlydearly <799511800@qq.com>
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.
LGTM
There is a new function added in the go1.21 standard library, which can make the code more concise and easy to read.