-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
CLI, maven, gradle: filter extensions by categories #17765
Conversation
import picocli.CommandLine; | ||
|
||
public class CategoryListFormatOptions { | ||
@CommandLine.Option(names = { "--name" }, order = 1, description = "Display category name only. (default)") |
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.
I was thinking if --name
format switch should rather be --id
, both for extensions and categories (and the same for maven and gradle). Cause it doesn't really show verbose names but artifactIds / category ids. It feels bit confusing to me, plus I don't know what to write in the description - "--name
displays category id". :)
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.
@aloubyansky ? I don't have an issue with switching to --id
devtools/cli/src/main/java/io/quarkus/cli/build/GradleRunner.java
Outdated
Show resolved
Hide resolved
devtools/cli/src/main/java/io/quarkus/cli/build/GradleRunner.java
Outdated
Show resolved
Hide resolved
devtools/cli/src/main/java/io/quarkus/cli/ProjectExtensions.java
Outdated
Show resolved
Hide resolved
devtools/cli/src/main/java/io/quarkus/cli/ProjectExtensionsCategories.java
Show resolved
Hide resolved
...rojects/tools/devtools-common/src/main/java/io/quarkus/devtools/commands/ListCategories.java
Show resolved
Hide resolved
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListCategoriesCommandHandler.java
Outdated
Show resolved
Hide resolved
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListCategoriesCommandHandler.java
Outdated
Show resolved
Hide resolved
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListExtensionsCommandHandler.java
Outdated
Show resolved
Hide resolved
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListExtensionsCommandHandler.java
Outdated
Show resolved
Hide resolved
@ebullient thanks for great review! I didn't manage to find more time today, I will get to it again on Monday. |
#17966 will intersect with this one, but I don't think they should disagree violently. They just touch some of the same files. Should be straight-forward to rebase on it alongside the other requested changes. |
1a63f09
to
839d503
Compare
This workflow status is outdated as a new workflow run has been triggered. |
I'm not sure if got your intentions regarding pushing the hints from handlers to gradle and maven plugins, so I pushed it as a separate commit. Going to squash later on if we agree on it. |
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListExtensionsCommandHandler.java
Outdated
Show resolved
Hide 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.
A few more comments, but way better. Make sure you rebase to pull in #17966. Note that the "list extensions for the current project" is important wording.. it isn't just generally existing categories, it has to do with whether or not the extensions being listed are relative to the platform used by the project, or based on some other factor (no project, or explicit stream/platform specified on command line). #17966 makes that meaning more obvious (I think).
...rojects/tools/devtools-common/src/main/java/io/quarkus/devtools/commands/ListCategories.java
Show resolved
Hide resolved
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListCategoriesCommandHandler.java
Outdated
Show resolved
Hide resolved
...common/src/main/java/io/quarkus/devtools/commands/handlers/ListExtensionsCommandHandler.java
Outdated
Show resolved
Hide resolved
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 839d503
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 Windows #📦 extensions/vertx-http/deployment✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ |
6031a05
to
b87d50a
Compare
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 6031a05
|
BTW moving all hints away from handlers to the CLI module doesn't work that well for the GradleRunner. This runner actually executes the
i.e. the hints are separated from the extensions list by plenty of gradle logging - they are hard to find and it doesn't looks sensible... For this reason I will try to make the CLI not to print any hints if gradle runner is used, and delegate that to the gradle task instead. It means there will be couple more conditions in gradle task, and the decoupling from CLI won't be perfect, but IMO it can't stay this way. |
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building b87d50a
|
I think most should be done. From my side I need to clarify: 1. The position of hints - which put before or after the extension / category list. Currently:
and
2. I didn't find how to obtain the executable name or alias which was used to run the CLI command, so I still use |
The command as defined by the CLI is I don't think it is right to have to go down into the bowels of the gradle plugin to fix a message about cli options (same for the maven plugin, or for the jbang plugin when it eventually supports listing things like this). As an option, instead of immediately returning at [2], we could store the result. [1]
[2]
|
43348a3
to
e55fc3c
Compare
That's what I already do... Simply calling the gradle runner you get the extensions list surrounded by some gradle noise from both the beginning and the end. But actually the gradle noise and the end is not as long as the noise at the beggining of the runner ouput. I was still thinking of having some hints at the beginning yesterday, but having hints at the end only looks OK (and I would prefer that anyway). Example:
|
I reverted the last part - hints are now printed from CLI / gradle runner / maven runner depending on which tool was executed by a user. |
e55fc3c
to
a534b9d
Compare
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.
One nit, and then squash to one commit, and we're good to go!
Thanks for sticking with this and working through feedback, I appreciate it! =)
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building a534b9d
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/scala✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/scala✖ ✖ ⚙️ JVM Tests - JDK 16 #📦 integration-tests/scala✖ ✖ |
* add ability to list extensions by category, * add command to list categories, * extensions ordered alphabetically by artifactId, * consistent maven property names (-Dformat rather than -Dquarkus.extensions.format), * --name switch renamed to --id (--name still works though)
a534b9d
to
feb2c6d
Compare
@ebullient thanks for your time reviewing this! Commits are squashed, but let me know should notice anything more... |
@ebullient please do not merge PRs with merge commits they are a pain to cherry-pick... |
sorry.. |
Fixes #16154