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

Various documentation fixes #666

Merged

Conversation

cescoffier
Copy link
Member

@cescoffier cescoffier commented Jan 29, 2019

@cescoffier cescoffier added this to the 0.8.0 milestone Jan 29, 2019
@cescoffier cescoffier requested a review from gsmet January 29, 2019 18:56
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.

Looks mostly good, added some comments inline.

hello shamrock!
----

TIP: If the application requires configuration values and these values are not set, an error is thrown.
Copy link
Member

Choose a reason for hiding this comment

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

A TIP admonition would make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's already an admonition (single line admonition).

Admonition blocks are supported too and allows adding title and paragraph as content. However, for short notes like this one I've found single-line more readable.

You can also generate the native executable with `mvn clean package -Pnative`
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this patch but maybe we should be consistent with what we do in Shamrock proper and use -Dnative. We would need the profile to be activated on the property.

Not sure it's worth the work though. In any case, if we decide to do it, let's do it in a follow-up PR (and update all the guides).

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to prefer specifying profiles explicitly, at least for the users. Now, if the user is a Maven user, he can definitely change that.

But we can change the behavior (it would require changing the template, doc, and quickstarts).

====
The events are also called in _dev mode_ between each redeployment.
====
TIP: The events are also called in _dev mode_ between each redeployment.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, one line admonitions are supported this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
Copy link
Member Author

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for the review. I've covered all the issue except one (the profile).
Let's track this one in a separate issue as it requires more coordination (doc, template, and quickstarts).

hello shamrock!
----

TIP: If the application requires configuration values and these values are not set, an error is thrown.
Copy link
Member Author

Choose a reason for hiding this comment

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

it's already an admonition (single line admonition).

Admonition blocks are supported too and allows adding title and paragraph as content. However, for short notes like this one I've found single-line more readable.

You can also generate the native executable with `mvn clean package -Pnative`
Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to prefer specifying profiles explicitly, at least for the users. Now, if the user is a Maven user, he can definitely change that.

But we can change the behavior (it would require changing the template, doc, and quickstarts).

====
The events are also called in _dev mode_ between each redeployment.
====
TIP: The events are also called in _dev mode_ between each redeployment.
Copy link
Member Author

Choose a reason for hiding this comment

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

docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
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.

Sorry, added 3 more comments after reviewing your changes. We're nearly there!


In your favorite IDE, create a new Maven project.
It should generate a `pom.xml` file with a content similar to:
The easiest way to create a new Protean project is to open a terminal and run the following command:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, missed that on the first review, but Protean should be {project-name}. We use that consistently in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn! Yes, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

replaced everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Have you pushed your changes? It still appears as "Protean" on GitHub?

Copy link
Member Author

Choose a reason for hiding this comment

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

well... pushed on the wrong remote. Will cleanup once merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

should be better now.

docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/getting-started-guide.adoc Outdated Show resolved Hide resolved
* use {project-name} instead of Protean
* rephrase the part on junit 5 and surefire
@cescoffier
Copy link
Member Author

I've fixed all of them! Thanks!

@cescoffier cescoffier merged commit 3249147 into quarkusio:master Jan 30, 2019
@cescoffier cescoffier deleted the features/various-documentation-fixes branch January 30, 2019 12:42
@cescoffier cescoffier added the kind/enhancement New feature or request label Jan 30, 2019
maxandersen added a commit to maxandersen/quarkus that referenced this pull request Nov 5, 2022
Co-authored-by: Stuart Douglas <stuart.w.douglas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert the getting started guide to use maven-shamrock-plugin:create
2 participants