quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.4k stars 2.57k forks source link

Generated app - deprecated className property highlighted instead of preferred packageName #14391

Open rsvoboda opened 3 years ago

rsvoboda commented 3 years ago

https://github.com/quarkusio/quarkus/blob/master/devtools/maven/src/main/java/io/quarkus/maven/CreateProjectMojo.java#L109 says that className parameter is deprecated and packageName should be used instead.

Quarkus guides are still using className parameter when generating sample project, e.g. https://quarkus.io/guides/getting-started#bootstrapping-the-project

cd /Users/rsvoboda/git/quarkus/docs
git grep className | wc -l
      69
git grep packageName | wc -l
       0

Mentioned change landed via https://github.com/quarkusio/quarkus/pull/13346 on Nov 18, 2020

There are some changes needed to be done, they are discussed on Zulip between @ia3andy and @maxandersen atm

ia3andy commented 3 years ago

I am implementing a fix:

I will remove the deprecation notice on className and add this behavior:

Using the 'className' option is restricting example code possibilities to 'resteasy', 'resteasy-reactive' and 'spring-web'
You can use 'packageName' to enjoy the full range of code examples...

This will fix all guides.

ia3andy commented 3 years ago

Here is the output:

mvn io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:create -Dextensions="neo4j, resteasy-jackson" -DclassName=com.meistermeier.neo4j.MovieResource -DprojectGroupId=com.meistermeier.neo4j -DprojectArtifactId=rest-neo4j-quarkus
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------------< org.apache.maven:standalone-pom >-------------------
[INFO] Building Maven Stub Project (No POM) 1
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- quarkus-maven-plugin:999-SNAPSHOT:create (default-cli) @ standalone-pom ---
[INFO] 
[INFO] ========================================================================================
[WARNING] Using the 'className' option is restricting example code possibilities to 'resteasy', 'resteasy-reactive' and 'spring-web'.
[WARNING] You can use 'packageName' to enjoy the full range of code examples...
[INFO] ========================================================================================
-----------
selected extensions: 
- io.quarkus:quarkus-neo4j
- io.quarkus:quarkus-resteasy-jackson

applying codestarts...
🔠 java
🧰 maven
🗃 quarkus
📜 config-properties
🛠 dockerfiles
🛠 maven-wrapper
🐒 resteasy-example

-----------
👍 quarkus project has been successfully generated in:
--> /Users/ia3andy/workspace/redhat/quarkus/demo/rest-neo4j-quarkus
-----------
[INFO] 
[INFO] ========================================================================================
[INFO] Your new application has been created in /Users/ia3andy/workspace/redhat/quarkus/demo/rest-neo4j-quarkus
[INFO] Navigate into this directory and launch your application with mvn quarkus:dev
[INFO] Your application will be accessible on http://localhost:8080
[INFO] ========================================================================================
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.792 s
[INFO] Finished at: 2021-01-19T18:01:26+01:00
[INFO] ------------------------------------------------------------------------
ia3andy commented 3 years ago

Ok I'll have the PR ready tomorrow :)

rsvoboda commented 3 years ago

Proposed warning in generator execution looks like a good start.

1) I would change the wording a bit:

[WARNING] You can use 'packageName' to enjoy the full range of code examples...
 =>
[WARNING] Use 'packageName' to have richer set of generated code examples.

2) What are your plan on guides side? packageName property needs to be introduced to users. To me this is the minimal set where the info about packageName property needs to be introduced

3) https://quarkus.io/guides/maven-tooling guide doesn't list packageName property at all in the table of available properties. (The following table lists the attributes you can pass to the create command:)

4) How will mvn io.quarkus:quarkus-maven-plugin:1.11.1.Final:create work in interactive mode? Will it ask about className parameter or about packageName parameter?

5) How can I figure out what extensions have examples are available (using cli)?

https://code.quarkus.io/ shows fighter jet icon when the extension has code examples, but I couldn't find a way how to list just extensions with code examples. Tried various keywords in search field (Pick your extensions) but nothing worked.

jsmrcka commented 3 years ago

It looks like there is an intention to move from using className to using packageName, in order to make the code examples more transparent. With that in mind, un-deprecating the className seems like a step back.

ia3andy commented 3 years ago

@rsvoboda

Proposed warning in generator execution looks like a good start.

  1. I would change the wording a bit:
[WARNING] You can use 'packageName' to enjoy the full range of code examples...
 =>
[WARNING] Use 'packageName' to have richer set of generated code examples.

👍

  1. What are your plan on guides side? packageName property needs to be introduced to users. To me this is the minimal set where the info about packageName property needs to be introduced

. quarkus.io/guides/maven-tooling guide doesn't list packageName property at all in the table of available properties.

Well, with the coming fix, I guess using className is making sense again, keeping in mind that only a few relevant examples are compatible. Those example are meant to prepare the ground of a real app while the other are more meant to be learning examples. This could be made even more intuitive in the future, but right now, the priority is to fix the bug and I can't work on all front.

I don't think we should promote the packageName option as it's using the groupId as package name by default. The packageName is here for the few who will need something different from the groupId..

@jsmrcka this should also answer your concern?

(The following table lists the attributes you can pass to the create command:)

  1. How will mvn io.quarkus:quarkus-maven-plugin:1.11.1.Final:create work in interactive mode? Will it ask about className parameter or about packageName parameter?

It's asking for the groupId like on code.quarkus.io

  1. How can I figure out what extensions have examples are available (using cli)?

code.quarkus.io shows fighter jet icon when the extension has code examples, but I couldn't find a way how to list just extensions with code examples. Tried various keywords in search field (Pick your extensions) but nothing worked.

Yeah we should create an issue for this, this is missing.

ia3andy commented 3 years ago

@maxandersen @FroMage it's been a while we are turning around this problem of different kind of codestarts and I think it's time to make it clear with names:

In the end, here in the fix, we are enabling the "skeleton mode" which restrict to specific compatible codestarts (resteasy, spring-web, and resteasy-reactive) when className is used.

Also, we should have a legend for this with different icons in our tooling.

maxandersen commented 3 years ago

I still think we are mixing up concerns here or maybe going to more advanced stages before we have the basics in place.

At the moment codestarts are used for an extension to add one codestart when users chooses to include that extension, right ?

choosing a extension does not add multiple codestarts - just one per extension. We don't have a mechnism to pick other codestarts, do we ?

Thus having that extension being paramaterized with className does not seem like a bad thing to have even though its just about example code.

so example code is = skeleton code with parameters.

Leaving just the issue with singletons where a codestart just doesn't make sense "merged" in - such as amazon lambda....why can't they just be called ie. "standalone example" and we are good?

ia3andy commented 3 years ago

I still think we are mixing up concerns here or maybe going to more advanced stages before we have the basics in place.

At the moment codestarts are used for an extension to add one codestart when users chooses to include that extension, right ?

I don't see how this is related?

choosing a extension does not add multiple codestarts - just one per extension. We don't have a mechnism to pick other codestarts, do we ?

Thus having that extension being paramaterized with className does not seem like a bad thing to have even though its just about example code.

Well, as explained, only a few examples are compatible with it, so we need to make this clear for the user and when used only one skeleton example can be activated (which seems logical as we only have one generic parameter).

TBH, there is another possibility but I am not really in favor as it will introduce a lot of documentation, complexity and possible conflicts. We could allow edition of some chosen codestarts data, then you can combine them (something like): -Dspring-web-example.resource.class-name=org.me.Aloha -Dresteasy-example.resource.class-name=org.me.Bloha -Dresteasy-example.resource.path=/foo -Dresteasy-example.resource.path=/bar

so example code is = skeleton code with parameters.

Leaving just the issue with singletons where a codestart just doesn't make sense "merged" in - such as amazon lambda....why can't they just be called ie. "standalone example" and we are good?

I don't exactly get what you mean here, but "standalone" example is fine to me, it's named singleton for now which is alright too IMO.

maxandersen commented 3 years ago

https://github.com/quarkusio/quarkus/blob/master/devtools/maven/src/main/java/io/quarkus/maven/CreateProjectMojo.java#L109 says that className parameter is deprecated and packageName should be used instead.

Quarkus guides are still using className parameter when generating sample project, e.g. quarkus.io/guides/getting-started#bootstrapping-the-project

just to be clear here the deprecation is at the code level. The guides will still use classname until we get worked through them all. It is not like things are stopping to work ;)

ia3andy commented 3 years ago

Ok so after a discussion with @maxandersen here is what we settled on:

  1. Keep the className @Deprecated (and forget about my fix proposal) and make path @deprecated. Using a generic className and path parameters with multiple example is not making sense anymore and lead to confusion. The groupId used as package by default (or the packageName for special cases) should be preferred. This until we provide a way to directly set specific example parameters (resteasy.class-name, resteasy.path, spring-web.class-name, ...) https://github.com/quarkusio/quarkus/issues/14437.
  2. Add a quick fix to the guides which are broken by adding resteasy in the list of extensions to make sure the code displayed in the guide is generated. It may generate more because some extensions may contains codestarts, but at least we are sure that the guide code is generated. Also it's ok if they use the @Deprecated className and path for a little while we are in a transition.
  3. Create an issue containing all different guides which could benefit of an already existing codestarts (like resteasy-jackson, qute, ...) to adapt them (and their quickstarts) https://github.com/quarkusio/quarkus/issues/14444.
  4. Add a new -Dexamples=resteasy-jackson to pick a specific example (or more) which would be cleaner (and more consistent) for guides, we won't need to manually pick extensions anymore in the command, they would already be provided by the example (#12957).

Let me know if you have objection or question :)

rsvoboda commented 3 years ago

Just one thing to clarify:

https://quarkus.io/guides/getting-started#bootstrapping-the-project should be changed from

mvn io.quarkus:quarkus-maven-plugin:1.11.0.Final:create \
    -DprojectGroupId=org.acme \
    -DprojectArtifactId=getting-started \
    -DclassName="org.acme.getting.started.GreetingResource" \
    -Dpath="/hello"

to

mvn io.quarkus:quarkus-maven-plugin:1.11.0.Final:create \
    -DprojectGroupId=org.acme \
    -DprojectArtifactId=getting-started \
    -Dpath="/hello"

Drop -DclassName="org.acme.getting.started.GreetingResource" to promote more the path when groupId is used as package by default.

ia3andy commented 3 years ago

@rsvoboda

I don't think we should drop it because this command is meant to generate an example consistent with the corresponding quickstart: https://github.com/quarkusio/quarkus-quickstarts/tree/master/getting-started

We could of course adapt the quickstarts, but I believe it would be better to transition toward this:

mvn io.quarkus:quarkus-maven-plugin:1.11.0.Final:create \
    -DprojectGroupId=org.acme.getting.started \
    -DprojectArtifactId=getting-started \
    -Dexamples=resteasy \
    -Dresteasy.className=GreetingResource \
    -Dresteasy.path="/hello"

(as soon as #14437 and https://github.com/quarkusio/quarkus/issues/12957 are ready)

ia3andy commented 3 years ago

As @maxandersen said, now that we have a clear plan for it, it's fine to keep using the @Deprecated className and path in the guides while transitioning.

ia3andy commented 2 years ago

Even with the recent changes, it is still relevant. We should find a way to define some codestart configs through the tooling (CLI, Maven plugin).

We could have a list of available configs:

resteasy-codestart.resource.path
resteasy-codestart.resource.class-name
resteasy-reactive-codestart.resource.path
...
spring-web-codestart.resource.path
...

Still, I think we should keep the className and path options as shortcuts (and remove the @Deprecated)

quarkus-bot[bot] commented 2 years ago

/cc @quarkusio/devtools

ia3andy commented 2 years ago

Also why aren't we using the Quarkus CLI in our doc?