-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Update jetty-maven-plugin.adoc #10810
Conversation
Update Jetty maven plugin for Jetty 12.
fix #10809 |
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.
Thanks a lot for your PR.
But we might need something more generic as the users may use ee8 or ee9 version
...tation/jetty-documentation/src/main/asciidoc/programming-guide/maven/jetty-maven-plugin.adoc
Outdated
Show resolved
Hide resolved
...tation/jetty-documentation/src/main/asciidoc/programming-guide/maven/jetty-maven-plugin.adoc
Outdated
Show resolved
Hide resolved
@olamy The doc is updated, please check it. |
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.
LGTM
Thanks
<groupId>org.eclipse.jetty</groupId> | ||
<artifactId>jetty-maven-plugin</artifactId> | ||
<groupId>org.eclipse.jetty.ee{8,9,10}</groupId> | ||
<artifactId>jetty-ee{8,9,10}-maven-plugin</artifactId> |
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 understand what you're trying to do here, but you won't believe how many people simply cut and paste from the documentation into their pom and then complain it doesn't work. So I think its fine to have {8,9,10}
in the body of the text, but any example should refer to ee10
only. So this example (and all others) would be:
<groupId>org.eclipse.jetty.ee10</groupId>
<artifactId>jetty-ee10-maven-plugin</artifactId>
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.
There are several ee{8,9,10}
, we need replace all of them to ee10
(as the initial version)?
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.
Really, I'm not kidding when I say people just cut and paste from the documentation without reading properly or engaging their critical faculties :)
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 think it's important to preface each example with your {8,9,10}
comment, and say that the example shows how to do it for ee10
.
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.
Updated.
...tation/jetty-documentation/src/main/asciidoc/programming-guide/maven/jetty-maven-plugin.adoc
Outdated
Show resolved
Hide resolved
...tation/jetty-documentation/src/main/asciidoc/programming-guide/maven/jetty-maven-plugin.adoc
Outdated
Show resolved
Hide resolved
…g-guide/maven/jetty-maven-plugin.adoc Co-authored-by: Jan Bartel <janb@webtide.com>
…g-guide/maven/jetty-maven-plugin.adoc Co-authored-by: Jan Bartel <janb@webtide.com>
@hantsy Thanks for your contribution |
Update Jetty maven plugin for Jetty 12.