-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Include Java example code excerpts #4642
Conversation
a785986
to
9bcc5c1
Compare
@svrnm can you help with the build error? |
@chalin can you take a look? Or should we currently not introduce more pages that use the code excerpt? |
Thanks for the ping and your patience. Great work here @zeitlinger! Could you rebase and resolve the conflicts and I'll take a look. I'm OOO next week, but try to work on this PR tomorrow if you can get the conflicts resolved by then.
I think that it is worth bring in this PR. Oh that build error was tricky to resolve. I can't recall how I worked through it the last time, but I'll find out when I next work on this PR. |
1b311e7
to
37ac4f6
Compare
@chalin merge conflict resolved |
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.
Great work here @zeitlinger!
I've fixed the build issue:
And I tweaked the position of directives so that they don't appear in auto-generated page description metadata.
Note that I haven't reviewed the content of this PR carefully. My main focus in this review was from a tooling/page generation perspective. But from that perspective, this PR LGTM ✨
@open-telemetry/java-approvers @open-telemetry/java-instrumentation-approvers PTAL |
Btw, @zeitlinger - the script supports many options that allow you to adjust the code excerpts that are pulled in, including the use of |
3c05d3e
to
020eaba
Compare
@chalin can you check? |
020eaba
to
107c750
Compare
@zeitlinger - do you mean check if named regions would be a useful feature to apply in this PR? I won't have the time to do that. I suggest that we get this merged first. We're still waiting for the following approvals from Java SIG approvers:
|
I actually like the fact that all files are complete and can be copied as is |
@zeitlinger can you check with Java SIG to have someone review and approve, then happy to 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.
This is cool! I'm not familiar with all of the mechanics of how code snippets are referenced / injected, but the code in the java snippets looks good to me.
@chalin added some tooling for that from the dart project. We are still in a "public beta" phase of that tooling, but it looks promising, but it's also still a manual process to get things updated, etc. Thanks for the review! |
Thank you @zeitlinger, @svrnm, et al. I'm glad that this got merged! |
No description provided.