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.

Sign up for GitHub

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

Jump to bottom

Implement the new selection algorithm #2785

Merged
merged 3 commits into from
Jun 19, 2019
Merged

Implement the new selection algorithm #2785

merged 3 commits into from
Jun 19, 2019

Conversation

cescoffier
Copy link
Member

Related to #2737.

@cescoffier cescoffier mentioned this pull request Jun 11, 2019
Merged
gsmet
gsmet reviewed Jun 11, 2019
View reviewed changes
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.

Ah, you beat me to it, thanks!

I added two minor comments. It looks good otherwise but I'll let @emmanuelbernard do the review.

devtools/common/src/test/java/io/quarkus/cli/commands/AddExtensionsTest.java Show resolved Hide resolved
docs/src/main/asciidoc/using-vertx.adoc Outdated Show resolved Hide resolved
@mkouba
Copy link
Contributor

mkouba commented Jun 11, 2019

Hm, I wonder if we should display the short name and possibly also tags in list-extensions (fulll format)?

emmanuelbernard
emmanuelbernard previously requested changes Jun 11, 2019
View reviewed changes
devtools/common/src/main/filtered/extensions.json Outdated Show resolved Hide resolved
devtools/common/src/main/filtered/extensions.json Outdated Show resolved Hide resolved
devtools/common/src/main/java/io/quarkus/cli/commands/AddExtensions.java Show resolved Hide resolved
devtools/common/src/main/java/io/quarkus/cli/commands/AddExtensions.java Outdated Show resolved Hide resolved
devtools/common/src/main/java/io/quarkus/cli/commands/AddExtensions.java Outdated Show resolved Hide resolved
devtools/common/src/main/java/io/quarkus/cli/commands/AddExtensions.java Outdated Show resolved Hide resolved
devtools/common/src/main/java/io/quarkus/cli/commands/AddExtensions.java Outdated Show resolved Hide resolved
devtools/common/src/main/java/io/quarkus/cli/commands/AddExtensions.java Outdated Show resolved Hide resolved
devtools/common/src/test/java/io/quarkus/cli/commands/AddExtensionsTest.java Outdated Show resolved Hide resolved
devtools/common/src/test/java/io/quarkus/cli/commands/AddExtensionsTest.java Show resolved Hide resolved
@emmanuelbernard
Copy link
Member

Hm, I wonder if we should display the short name and possibly also tags in list-extensions (fulll format)?

list-extensions is already too long. I'd use shortname and labels in the web ui as tags and as search restrictions. Not necessarily expose them.

@cescoffier cescoffier added area/maven kind/enhancement New feature or request labels Jun 12, 2019
@cescoffier
Copy link
Member Author

If you find conflicts or odd behavior in the currently used algorithm (in 0.16.1), let me know we can add a test and check.

@gsmet gsmet added this to the 0.17.0 milestone Jun 17, 2019
@gsmet
Copy link
Member

gsmet commented Jun 17, 2019 •
edited
Loading

@cescoffier could you squash things a bit? Typically the Apply formater commit should go away. As for the others, I let you judge if they have values. I think I would squash everything to have one commit with all the tuning but feel free to do otherwise.

@cescoffier
Copy link
Member Author

@gsmet I would squash this into a single commit no? The different iterations are not very meaningful. (Github can to it).

@emmanuelbernard
Copy link
Member

I looked fast but I did not see implemented the approach where the short name was used as the default value for a given choice menu.
In #2737 (comment) it's described as option 2.

Were do we stand on option 1 vs option 2 collectively. We we discussed it by voice, I think the compromise was for option 2 as option 1 was denying the notion of a kafka label for searches.

@cescoffier
Copy link
Member Author

cescoffier commented Jun 19, 2019 •
edited
Loading

Option 2 requires a prompt and changing completely how the Maven plugin works (2 phases selections and so on). That's why I picked 1 for now until we find the right approach to implement 2.

@gsmet gsmet modified the milestones: 0.17.0, 0.18.0 Jun 19, 2019
@gsmet
Copy link
Member

gsmet commented Jun 19, 2019

Moving to 0.18.0. I discussed with @cescoffier and the situation is unclear. I think we need a call between @emmanuelbernard and @cescoffier to clarify the situation and converge.

@FroMage
Copy link
Member

FroMage commented Jun 19, 2019

Also needs to be rebased now, apparently.

@FroMage FroMage added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Jun 19, 2019
@gsmet gsmet modified the milestones: 0.18.0, 0.17.0 Jun 19, 2019
@cescoffier
Implementation of the new extension selection algorithm as proposed in …
1891574 
…quarkusio#2737.

Impact of the new selection algorithm on the guides.

Most of it is purely cosmetic and allow using shorter names. Some tweaks are required to avoid ambiguity.

Do not use dash in bean validation.

Remove the swagger-ui short name. Swagger may mean something else.
@cescoffier
Support partial matches in the name, artifactId or shortName
55f73d6 
If a query is a GAV, insert the dependency as a GAV. Also, avoid reloading the extension list every time.

When the query is containg in the artifact id or in the name of the extension.
@cescoffier
Add short names for smallrye dependencies in the extension list
cc50c1b 
Disable the label match by default
@cescoffier cescoffier force-pushed the features/new-extension-selection-algorithm branch from 6667028 to cc50c1b Compare June 19, 2019 10:42
@cescoffier cescoffier removed the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Jun 19, 2019
@cescoffier
Copy link
Member Author

Commit squashed, rebase done.

@gsmet gsmet dismissed emmanuelbernard’s stale review June 19, 2019 11:34

Emmanuel agreed we should merge this one for 0.17.0.

@gsmet gsmet merged commit 166f98e into quarkusio:master Jun 19, 2019
@cescoffier cescoffier deleted the features/new-extension-selection-algorithm branch June 19, 2019 12:03
@Dufgui Dufgui mentioned this pull request Aug 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenfinnigan kenfinnigan kenfinnigan left review comments

@emmanuelbernard emmanuelbernard emmanuelbernard left review comments

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
area/maven kind/enhancement New feature or request
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.
6 participants
@cescoffier @mkouba @emmanuelbernard @gsmet @FroMage @kenfinnigan