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: Client improvements #6

Merged
merged 11 commits into from
Sep 20, 2023
Merged

feat: Client improvements #6

merged 11 commits into from
Sep 20, 2023

Conversation

slowli
Copy link
Collaborator

@slowli slowli commented Sep 20, 2023

What ❔

Improves the client crate in various ways:

  • Supports Gauges for more types, e.g. usize / isize.
  • Supports raw identifiers (e.g., r#type) as label names.
  • Supports encoding C-style enums as label values and transforming their variant names like in serde.

Why ❔

Makes the crate easier to use (with the guiding example being the main Era repo).

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • Code has been formatted and linted using cargo fmt and cargo clippy.

@slowli slowli marked this pull request as ready for review September 20, 2023 10:17

let mut output = String::with_capacity(ident.len());
for (i, ch) in ident.char_indices() {
if i > 0 && ch.is_ascii_uppercase() {
Copy link
Member

Choose a reason for hiding this comment

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

Do I get it right that this assumes that the ident is always in UpperCamelCase?
If so, is it also expected that acronyms are always treated like words?
E.g. looked like this would transform HTTPStatus to h_t_t_p_status via SnakeCase.

Copy link
Member

Choose a reason for hiding this comment

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

Also, do we want to break on digits? E.g. Status500 -> status_500?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've consulted the serde impl 🙃 It does have the peculiarities you've described:

  • It assumes that the input is PascalCase. IMO, it's a reasonable assumption to make; I guess we can document it.
  • Acronyms are tricky. IIRC, Rust best practices recommend to capitalize only the first letter of acronyms (i.e., HttpStatus rather than HTTPStatus). In any case, in these situations one can specify the name override explicitly.
  • Breaking on digits is tricky as well; I feel there's no obvious way to insert or not insert a _ before them. Again, it's possible to specify the name override to make it explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't suggest changing anything, just wanted to check whether this was considered.
Probably it indeed makes sense to document this behavior.

IIRC, Rust best practices recommend to capitalize only the first letter of acronyms

Well, yeah, there is a clippy lint for that, but it's ranked 4th in the list of most commonly ignored lints. Looks like the community doesn't agree with this best practice :)

@slowli slowli merged commit 1ee530c into main Sep 20, 2023
2 checks passed
@slowli slowli deleted the client-improvements branch September 20, 2023 12:42
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.

2 participants