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

Include Java example code excerpts #4642

Merged
merged 10 commits into from
Jul 24, 2024
Merged

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Jun 10, 2024

No description provided.

.gitmodules Outdated Show resolved Hide resolved
@zeitlinger zeitlinger marked this pull request as ready for review June 19, 2024 12:44
@zeitlinger zeitlinger requested review from a team June 19, 2024 12:44
@zeitlinger
Copy link
Member Author

@svrnm can you help with the build error?

@svrnm
Copy link
Member

svrnm commented Jun 19, 2024

@svrnm can you help with the build error?

@chalin knows best how the code excerpts work, let's wait for him to be back and take a look!

@svrnm
Copy link
Member

svrnm commented Jul 11, 2024

@chalin can you take a look? Or should we currently not introduce more pages that use the code excerpt?

@chalin
Copy link
Contributor

chalin commented Jul 11, 2024

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.

@chalin can you take a look? Or should we currently not introduce more pages that use the code excerpt?

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.

@zeitlinger
Copy link
Member Author

@chalin merge conflict resolved

@chalin chalin changed the title include Java examples Include Java example code excerpts Jul 12, 2024
Copy link
Contributor

@chalin chalin left a 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:

image

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 ✨

@chalin
Copy link
Contributor

chalin commented Jul 12, 2024

@open-telemetry/java-approvers @open-telemetry/java-instrumentation-approvers PTAL

@chalin
Copy link
Contributor

chalin commented Jul 12, 2024

Btw, @zeitlinger - the script supports many options that allow you to adjust the code excerpts that are pulled in, including the use of from and to regexs. You can also named regions in the code and pull in named regions only. The nice thing about named regions is that they don't even need to be contiguous: e.g., you can excerpt part of one function here, and another function there. But what you have here seems quite fine for now, I just wanted to mention some of the direction's options in case you didn't know about them.

@zeitlinger
Copy link
Member Author

@chalin can you check?

@chalin
Copy link
Contributor

chalin commented Jul 23, 2024

@chalin can you check?

@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:

  • @open-telemetry/java-approvers
  • @open-telemetry/java-instrumentation-approvers

@zeitlinger
Copy link
Member Author

@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.

I actually like the fact that all files are complete and can be copied as is

@svrnm svrnm added the sig-approval-missing Co-owning SIG didn't provide an approval label Jul 24, 2024
@svrnm
Copy link
Member

svrnm commented Jul 24, 2024

@zeitlinger can you check with Java SIG to have someone review and approve, then happy to merge this:-)

Copy link
Member

@jack-berg jack-berg left a 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.

@svrnm
Copy link
Member

svrnm commented Jul 24, 2024

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!

@svrnm svrnm merged commit 99dfcde into open-telemetry:main Jul 24, 2024
16 of 17 checks passed
@chalin
Copy link
Contributor

chalin commented Jul 24, 2024

Thank you @zeitlinger, @svrnm, et al. I'm glad that this got merged!
My plan, in the next couple of months I'll work on simplifying the tool chain (FYI)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig:java sig-approval-missing Co-owning SIG didn't provide an approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants