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

[feature] Weaver needs to update to_XYZ_case helpers to match namespacing rules in semconv #198

Closed
jsuereth opened this issue Jun 5, 2024 · 3 comments · Fixed by #200
Closed
Assignees

Comments

@jsuereth
Copy link
Contributor

jsuereth commented Jun 5, 2024

See See open-telemetry/semantic-conventions#1118 for context

@lquerel lquerel self-assigned this Jun 5, 2024
@lquerel
Copy link
Contributor

lquerel commented Jun 6, 2024

I see two options to implement this:

  1. Create a const_name filter that by default applies the namespacing rules and generates a SCREAMING_SNAKE_CASE ID. An optional case parameter will allow specifying another case converter after applying the namespacing rules.
  2. Add an optional parameter to all existing case converters (i.e. pascal_case, screaming_snake_case. ...) to specify whether the conversion should take into account the namespacing rules. We need to find a short name for this flag.

@jsuereth
Copy link
Contributor Author

jsuereth commented Jun 6, 2024

I think 1. isn't practical because constants vary in definition. If const_name took the case type as an argument, then I think that'd be viable.

  1. I like this, but it's also a bit of a footgun. I think it'd be more comfortable with semconv_const_name(case="Pascal")

@lquerel
Copy link
Contributor

lquerel commented Jun 6, 2024

@jsuereth I'm confused :-)
So in conclusion, you prefer option 1, but you'd like to rename const_name to semconv_const_name or semconv_const. Right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
2 participants