Nuance the message about filename-based automodules detected on the module path#11857
Nuance the message about filename-based automodules detected on the module path#11857desruisseaux wants to merge 2 commits into
Conversation
…odule path. Instead of "Please don't publish this project to a public artifact repository", said "This project may not work in applications that do not use these dependencies with exactly the same filenames as listed above".
| @@ -236,7 +236,7 @@ public JavaPathType getPathType() { | |||
| /** | |||
| * If the module has no name, adds the filename of the JAR file in the given collection. | |||
| * This method should be invoked for dependencies placed on {@link JavaPathType#MODULES} | |||
There was a problem hiding this comment.
not new here, but this doc comment really is hard to follow or understand. It should be rewritten.
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
The rationale is sound: the old message was overly prescriptive ("don't publish") when in practice Maven artifact filenames are deterministic. The new wording correctly shifts from a prohibition to an informational warning about the fragility.
Two minor observations below.
| * If the module has no name, adds the filename of the JAR file in the given collection. | ||
| * This method should be invoked for dependencies placed on {@link JavaPathType#MODULES} | ||
| * for preparing a warning asking to not deploy the build artifact on a public repository. | ||
| * for preparing a warning saying that this project may fail to resolve these modules. |
There was a problem hiding this comment.
Nit: the Javadoc on this method still says "for preparing a warning asking to not deploy the build artifact on a public repository", which no longer matches what the warning actually says. Should be updated to stay consistent with the new message.
| * for preparing a warning saying that this project may fail to resolve these modules. | |
| * for preparing a warning saying that this project may fail to resolve these modules. |
gnodet
left a comment
There was a problem hiding this comment.
LGTM — the new wording is a clear improvement. Minor nits noted inline.
Claude Code on behalf of Guillaume Nodet
|
Claude Code on behalf of Guillaume Nodet I've pushed a follow-up commit to address the review suggestion about the warning message wording. The commit simplifies the double-negative phrasing: Before:
After:
The Javadoc update in The commit is available at Or I can merge this into the main repo directly if you enable "Allow edits from maintainers" on this PR. |
Co-authored-by: Guillaume Nodet <gnodet@gmail.com>
|
I couldn't find the "Allow edits from maintainers" checkbox. Even the GitHub documentation does not match what I see. I will cherry pick. Thanks! |
Instead of "Please don't publish this project to a public artifact repository", said "This project may not work in applications that do not use these dependencies with exactly the same filenames as listed above".
Rational: it can be considered okay to publish the project on a public Maven repository, because the artifact filenames are quite deterministic with Maven. For a pure-Maven project and for bundle generated by the Maven assembly plugin, there is no reason why these filenames would not be stable. Therefore, we can warn the users about the fragility of filename-based automodules but let them decide whether they want to publish anyway.