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

[MNG-7571] add Maven API javadoc #830

Merged
merged 2 commits into from
Dec 31, 2022
Merged

[MNG-7571] add Maven API javadoc #830

merged 2 commits into from
Dec 31, 2022

Conversation

hboutemy
Copy link
Member

@hboutemy hboutemy commented Oct 17, 2022 •
edited
Loading

@hboutemy hboutemy marked this pull request as draft October 17, 2022 06:15
@gnodet gnodet added this to the 4.0.0-alpha-3 milestone Oct 24, 2022
@gnodet gnodet modified the milestones: 4.0.0-alpha-3, 4.0.0-alpha-4 Dec 1, 2022
@asfgit asfgit force-pushed the MNG-7571 branch 4 times, most recently from a4b5f92 to 522f7a4 Compare December 27, 2022 17:13
@hboutemy hboutemy mentioned this pull request Dec 27, 2022
Merged
@asfgit asfgit force-pushed the MNG-7571 branch 2 times, most recently from f565ba3 to 5afe135 Compare December 27, 2022 23:16
@hboutemy hboutemy requested review from gnodet and michael-o December 28, 2022 16:38
michael-o
michael-o requested changes Dec 28, 2022
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Mostly stylistic issues.

An general issue on separation: <name>Maven 4 API - Toolchain</name>

Stylistically a hyphen is the wrong symbol. It should be an en dash in this case, but many of our components use ::. See Doxia, Wagon, Core ITs, Indexer. So I would make it consistent here: ::

api/maven-api-core/src/site/site.xml Outdated Show resolved Hide resolved
api/maven-api-meta/pom.xml Outdated Show resolved Hide resolved
api/maven-api-meta/pom.xml
@@ -27,6 +27,7 @@
</parent>

<artifactId>maven-api-meta</artifactId>
<name>Maven API Meta annotations</name>
<name>Maven 4 API Meta annotations</name>
Copy link
Member

Choose a reason for hiding this comment

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

Annotations

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 we rename the artifactId?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good idea: maven-api-meta-annotations?

api/maven-api-meta/src/site/site.xml Outdated Show resolved Hide resolved
api/maven-api-model/pom.xml Outdated Show resolved Hide resolved
api/src/site/site.xml Outdated Show resolved Hide resolved
maven-toolchain-builder/src/site/site.xml Outdated Show resolved Hide resolved
maven-toolchain-model/src/site/site.xml Outdated Show resolved Hide resolved
maven-xml-impl/pom.xml Show resolved Hide resolved
plexus-utils/pom.xml
@@ -28,6 +28,8 @@ under the License.
<groupId>org.apache.maven</groupId>
<artifactId>plexus-utils</artifactId>
<version>4.0.0-alpha-4-SNAPSHOT</version>
<name>Apache Maven Plexus-Utils</name>
<description>Apache Maven repackaging of Plexus Utils with immutable Dom interface and its replacement implementation.</description>
Copy link
Member

Choose a reason for hiding this comment

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

DOM

Copy link
Member Author

Choose a reason for hiding this comment

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

no, the interface is named Dom, which is not the best name IMHO...

Copy link
Member

Choose a reason for hiding this comment

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

Really? This is confusing, it thought it is an acronym referring to Document Object Model...

Copy link
Contributor

Choose a reason for hiding this comment

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

The name comes from the plexus class Xpp3Dom but I removed the Xpp3 part. And yes, it's referring to Document Object Model, but I would not call it DOM. We could rename it XmlNode ?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable

@hboutemy
Copy link
Member Author

I'll merge this PR regarding basic documentation improvements

on the renaming ideas (Dom interface to XmlNode, maven-api-meta to maven-api-annotations), we'll do that separately while doing the plan discussed on dev ML https://lists.apache.org/thread/t52x7jfqlfxwtftbwv4vpfhqg6zgsysf

@hboutemy hboutemy marked this pull request as ready for review December 31, 2022 16:15
@hboutemy hboutemy merged commit fbdf109 into master Dec 31, 2022
@hboutemy hboutemy deleted the MNG-7571 branch December 31, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o requested changes

@gnodet gnodet Awaiting requested review from gnodet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0-alpha-4
Development

Successfully merging this pull request may close these issues.
3 participants
@hboutemy @gnodet @michael-o