Adds an option to render briefs and notes of semconv attribute groups in md #213
Conversation
…ups in markdown Signed-off-by: Alexander Wert <alexander.wert@elastic.co>
semantic-conventions/src/tests/data/markdown/attribute_group/attribute_group.yaml
Outdated
Show resolved
Hide resolved
semantic-conventions/src/tests/data/markdown/attribute_group/expected.md
Outdated
Show resolved
Hide resolved
| @@ -206,6 +210,12 @@ def write_table_header(self, output: io.StringIO): | |||
| def to_markdown_attribute_table( | |||
| self, semconv: BaseSemanticConvention, output: io.StringIO | |||
| ): | |||
| if self.render_ctx.is_render_group_brief: | |||
| output.write(semconv.brief + "\n\n") | |||
There was a problem hiding this comment.
Is there a case when we don't want to render brief and note? I'd prefer it to be a default (and maybe even not configurable).
Also, I'm not sure if we support multiple configurations (like full, tags and render_group_brief). If not, we'd have a harder time using this in semconv repo.
There was a problem hiding this comment.
Is there a case when we don't want to render brief and note? I'd prefer it to be a default (and maybe even not configurable).
I can imagine cases where we might want to only render the table without brief and note. So, I think having it being configurable is a good thing.
I'm up for making it enabled by default, though! Though, that would mean that all the existing markdown files would change once we update to a new version of the build-tools that would include that feature. And some of the briefs are currently duplicated (copy pasted) manually into the markdown files. So managing that change in a clean way would be very cumbersome if we would make it suddenly enabled by default.
What I can imagine could work quite well is:
- make it an opt-in config option first
- then start migrating (one by one) all the semconvs / markdown files to explicitly include the rendering of briefs and notes
- once all the semconvs / markdown files are migrated change the tooling to make that option enabled by default
Also, I'm not sure if we support multiple configurations (like full, tags and render_group_brief). If not, we'd have a harder time using this in semconv repo.
Yes, IIUC that's possible. You can specify multiple configurations at once (here's an example). Some of the config options are logically mutually exclusive / contradicting , though, when used at once (like full and remove_constraints).
Maybe we should make the rendering of briefs and notes enabled when the full option is being used, and otherwise an opt-in? @lmolkova WDYT?
There was a problem hiding this comment.
@lmolkova Any thoughts on the above?
There was a problem hiding this comment.
IMHO:
- It should not be separately controllable. Just have one single option for both brief+note.
- It should be on by default eventually
- I wonder if a command line option wouldn't be better here instead of a per-table option, as I see it, it is mostly only for backwards compatibility that we have this option.
There was a problem hiding this comment.
@lmolkova Any thoughts on the above?
I see your point @AlexanderWert, thanks for the explanation. I don't really see a strong reason to go through all the steps to start populating briefs/notes and would rather:
- populate them by default right away. then send a big diff PR with just this change. If some notes/briefs are too bad, either fix them right away or disable them.
- keep an option to not populate them on specific tables. Agree with @Oberon00 that one option for both should probably be enough
WDYT?
There was a problem hiding this comment.
The suggestions by @lmolkova is fine for me as well, that's the "more aggressive" option. So, always-on, no new options at all (neither command line nor per-table). Probably the most clean way.
…ttribute_group.yaml Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
…xpected.md Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
Fixes #15