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

Don't render deprecated enum members in semconv tables #1110

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jun 2, 2024

Came up in ##1006 (comment)

Deprecated enum members are not useful in semconv docs and should only appear in the registry.

Merge requirement checklist

@lmolkova lmolkova requested review from a team June 2, 2024 21:15
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

I wonder if we'd rather preserve the deprecated ones in the semconv tables but only for limited period of time, i.e. for 2 releases before removing them.
However I don't know if that would be technically doable. Maybe if we were encoding the version that a member/attribute was deprecated, deprecated-1.26.0.

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 3, 2024

I wonder if we'd rather preserve the deprecated ones in the semconv tables but only for limited period of time, i.e. for 2 releases before removing them. However I don't know if that would be technically doable. Maybe if we were encoding the version that a member/attribute was deprecated, deprecated-1.26.0.

You're right, we don't have a way to express in which version a property was deprecated. It came up in the past and I created #1117 to track it. If we had an option to remove them after X releases, I'd be happy to keep them around for a bit.

Until we have this option, I'd prefer not to render them in the specific semconvs - that's what we do for the attributes.

@lmolkova lmolkova merged commit c80cd1f into open-telemetry:main Jun 3, 2024
12 checks passed
@@ -1,10 +1,11 @@
{% import 'stability.j2' as stability %}
{% macro filter(member) %}{% if snippet_type is not defined or (member.deprecated is none or member.deprecated == "") %}{{ "True" }}{% else %}{{ "False" }}{% endif %}{% endmacro %}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I might be late to the review, but filter usually filters things out, so I was stumbled upon usage, where we filter member but still continue to process it later
So semantically it should be False for good member and True for deprecated one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% macro filter(member) %}{% if snippet_type is not defined or (member.deprecated is none or member.deprecated == "") %}{{ "True" }}{% else %}{{ "False" }}{% endif %}{% endmacro %}
{% macro filter_deprecated(member) %}{% if snippet_type is not defined or (member.deprecated is none or member.deprecated == "") %}{{ "False" }}{% else %}{{ "True" }}{% endif %}{% endmacro %}

something like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants