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

Add documentation for vertx and qute integration #41558

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

mcruzdev
Copy link
Contributor

@mcruzdev mcruzdev commented Jun 28, 2024

Fixes #41559

@mcruzdev mcruzdev marked this pull request as ready for review June 28, 2024 18:54
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 1, 2024
Copy link

github-actions bot commented Jul 1, 2024

🎊 PR Preview 322d112 has been successfully built and deployed to https://quarkus-pr-main-41558-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for this addition. I added a few minor comments.

But that being said, I'm a bit puzzled. Why would you have io.vertx.core.json.JsonObject in your classpath, if you don't have the quarkus-vertx extension?

At least in the Quarkus world, that seems like looking for weird issues as Vert.x won't work very well if you don't use the extension.

Could you explain how you stumbled upon this issue and why you wanted to document it?

@mkouba @geoand WDYT?

</dependency>
----

With `io.quarkus.vertx` capability we have a special value resolver for `io.vertx.core.json.JsonObject` which makes it possible to access the properties of a JSON object in a template:
Copy link
Member

Choose a reason for hiding this comment

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

With io.quarkus.vertx capability

I think this part is a bit too technical. The fact that we use capabilities is internal.
Maybe With this dependency included, ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, With this dependency included, is better but the "capability" here does not necessarily mean io.quarkus.deployment.Capability... I mean, it's ok to use this word with its original meaning.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're twisting what I'm saying :).

Using the word capability is fine. But here we are talking about io.quarkus.vertx capability which is the name of the capability (in the Quarkus meaning of it) thus too technical for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought there's quarkus-vertx... ok then and sorry.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, talking about capabilities makes sense when in the context of extension development. So if you're targeting this (and given what you said in another comment, it might be the case), then it makes sense to talk about capabilities.

But again, probably missing some context about when you actually need this.

Copy link
Contributor

Choose a reason for hiding this comment

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

And only keep this one (plus the example bellow) with the new wording suggested by Guillaume.

docs/src/main/asciidoc/qute-reference.adoc Outdated Show resolved Hide resolved
Comment on lines 2657 to 2670
[source,xml]
----
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-vertx</artifactId>
</dependency>
----
Copy link
Member

Choose a reason for hiding this comment

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

Here we should use the tabs with Gradle option too. There are numerous examples in the doc (for instance in validation.adoc).

Copy link
Contributor

Choose a reason for hiding this comment

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

And the same applies here - remove the GAV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the same applies here - remove the GAV.

Why remove GAV @mkouba ?

docs/src/main/asciidoc/qute-reference.adoc Outdated Show resolved Hide resolved
@mkouba
Copy link
Contributor

mkouba commented Jul 1, 2024

Thanks for this addition. I added a few minor comments.

But that being said, I'm a bit puzzled. Why would you have io.vertx.core.json.JsonObject in your classpath, if you don't have the quarkus-vertx extension?

At least in the Quarkus world, that seems like looking for weird issues as Vert.x won't work very well if you don't use the extension.

Could you explain how you stumbled upon this issue and why you wanted to document it?

@mkouba @geoand WDYT?

@gsmet It's a feature requested from https://github.com/quarkiverse/quarkus-roq. The idea is to add the Qute value resolver only if quarkus-vertx is on the class path. We use the same approach for quarkus-cache.

See #41390 for more information.

@gsmet
Copy link
Member

gsmet commented Jul 1, 2024

@mkouba well, it's your component so your decision but by reading this paragraph, I'm under the impression that, as a Quarkus user, I have to do something on my side when I want to have support for JsonObject.
But, except if you are actually dealing with conditional dependencies, you actually don't need to do anything.

I think this is a bit confusing and adding complexity for something that is an advanced usage and a corner case. It's probably worth documenting it but I would clearly explain the context and when this is actually needed. Because the fact that you needed to actually explain it to me is a sign that it's easy to miss. And I'm not exactly your typical Quarkus user :).

Feel free to dismiss my review once the Vert.x typo is fixed. I might be overthinking it :).

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 1, 2024
@quarkus-bot

This comment has been minimized.

@mcruzdev
Copy link
Contributor Author

mcruzdev commented Jul 1, 2024

Thank you for all comments and considerations!

@quarkus-bot

This comment has been minimized.

@mkouba mkouba requested a review from gsmet July 3, 2024 07:10
@mcruzdev mcruzdev force-pushed the vertx-integration-docs branch 2 times, most recently from 608a5aa to 7b28b60 Compare July 3, 2024 23:02
@mcruzdev
Copy link
Contributor Author

Hi @gsmet, could you make a new review?

@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented Aug 14, 2024

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@mcruzdev
Copy link
Contributor Author

mcruzdev commented Sep 3, 2024

Hi @gsmet, could you take a new review?

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 8, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 8aeaed6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gsmet gsmet merged commit 2014fc4 into quarkusio:main Sep 9, 2024
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 9, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Sep 9, 2024
@gsmet gsmet modified the milestones: 3.16 - main, 3.14.3 Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Add documentation for Vertx and Qute integration
4 participants