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

feat(encoding): impl EncodeLabelValue for bool #237

Merged

Conversation

koushiro
Copy link
Contributor

  • impl EncodeLabelValue for bool
  • adjust order of some impls (just for better consistency and readability)

@koushiro koushiro force-pushed the impl-encode-label-value-for-bool branch 2 times, most recently from 0835246 to b90a1d8 Compare November 10, 2024 13:16
Comment on lines +540 to +544
impl EncodeLabelValue for bool {
fn encode(&self, encoder: &mut LabelValueEncoder) -> Result<(), std::fmt::Error> {
encoder.write_str(if *self { "true" } else { "false" })
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

for the record, i would support this addition, i think it's a pleasant and natural addition to the library.

that said, the reordering of impl blocks here is probably best left for a distinct pull request, as it introduces a lot of undue noise for reviewers to inspect.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thank you.

Can you please add a changelog entry to the commit adding the impl for bool.

Please keep the split into two commits.

@koushiro koushiro force-pushed the impl-encode-label-value-for-bool branch from b90a1d8 to 6898722 Compare November 29, 2024 07:51
@koushiro koushiro requested a review from mxinden November 29, 2024 07:51
@koushiro
Copy link
Contributor Author

There are some clippy warnings introduced by rust 1.83 during CI check.
Fixed by #250

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks!

One nit pick. Otherwise ready to merge.

Mind updating to latest master, or giving me access to merge master into this pull request?

CHANGELOG.md Outdated Show resolved Hide resolved
@koushiro koushiro force-pushed the impl-encode-label-value-for-bool branch from 6898722 to 96796bc Compare November 29, 2024 10:27
@koushiro koushiro requested a review from mxinden November 29, 2024 11:34
@mxinden mxinden merged commit 7413b61 into prometheus:master Nov 29, 2024
10 checks passed
@koushiro koushiro deleted the impl-encode-label-value-for-bool branch November 29, 2024 17:55
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.

3 participants