-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add int and double types. Small bugfixes. #30
Conversation
semantic-conventions/src/opentelemetry/semconv/model/semantic_attribute.py
Show resolved
Hide resolved
semantic-conventions/src/opentelemetry/semconv/model/semantic_attribute.py
Show resolved
Hide resolved
"string", | ||
"string[]", | ||
"number", | ||
"number[]", | ||
"int", | ||
"int[]", | ||
"double", | ||
"double[]", | ||
"boolean", | ||
"boolean[]", |
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.
If you make the dictionary in type_mapper a (static) constant, you can just do if attr_type in that_dict
(in
checks the keys with a dictionary right-hand side)
@open-telemetry/specs-approvers Can we merge this? |
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.
@thisthat I think this is our first breaking change with the generator. Could you please either add a changelog section in the README or a separate file for tracking these changes? Users will have to update their YAML files accordingly.
@@ -275,17 +282,21 @@ def is_simple_type(attr_type: str): | |||
return attr_type in ( | |||
"string", | |||
"string[]", | |||
"number", | |||
"number[]", | |||
"int", |
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.
will it break all active PRs? Maybe keep number, than fix yamls and then remove number?
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.
This won't break active PRs until we bump the tool version in the Github action
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, Giovanni!
Co-authored-by: Armin Ruech <[email protected]>
This was recently introduced in open-telemetry/build-tools#30. Fixes open-telemetry#1096.
This PR fixes #13
FYI @anuraaga :)