Conversation
…on.md - Update phrasing from 'Contrary to... those files' to 'Unlike... these files' - Remove possessive from 'Maven's DefaultRequestCache' for consistency - Populate empty sections for Settings and Extensions with factual content
gnodet
left a comment
There was a problem hiding this comment.
The text quality fixes are good improvements — the phrasing correction ("Unlike standard properties files, these files") and the possessive removal are both clean.
Two documentation accuracy observations on the new Settings and Extensions content:
-
Property name:
${maven.conf}is not a canonical Maven 4 property. The correct property is${maven.installation.conf}(defined inConstants.javaasMAVEN_INSTALLATION_CONF, default${maven.home}/conf). The shippedmaven-system.propertiesalso usesmaven.installation.conf. Whilemaven.confexists as a legacy fallback for backward compatibility, the Maven 4 documentation should reference the canonical name. -
Missing project-level configuration: Maven 4 defines three configuration levels for both settings and extensions — installation, user, and project (
${maven.project.conf}/settings.xml→.mvn/settings.xml, and${maven.project.conf}/extensions.xml→.mvn/extensions.xml). These are defined inConstants.java(MAVEN_PROJECT_SETTINGS,MAVEN_PROJECT_EXTENSIONS) and configured in the shippedmaven-system.properties. The PR currently only documents two of the three levels.
Also, the PR body references "Fixes #5, #6, #7, #8" but those are unrelated, already-closed issues from 2013-2014.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
Corrected the reference to the Maven installation configuration path and updated the environment variable terminology.
There was a problem hiding this comment.
Pull request overview
Improves the Maven site documentation wording and fills in previously empty sections related to configuration locations, aiming to make configuration guidance clearer and more consistent across docs.
Changes:
- Refines wording around Maven properties file encoding and
MAVEN_ARGSterminology inconfiguring.md. - Adds introductory content for Settings and Extensions configuration in
configuring.md. - Adjusts phrasing in
cache-configuration.mdto remove possessive wording aroundDefaultRequestCache.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/site/markdown/configuring.md | Updates phrasing and adds Settings/Extensions configuration guidance. |
| src/site/markdown/cache-configuration.md | Tweaks introductory sentence wording for cache configuration docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gnodet
left a comment
There was a problem hiding this comment.
The new commit correctly addresses the prior review feedback:
${maven.conf}→${maven.installation.conf}, matchingConstants.MAVEN_INSTALLATION_CONF- "environment value" → "environment variable" — correct terminology
- "Those" → "These" — appropriate for adjacent context
Nice cleanup. The text quality fixes and property name correction are solid improvements.
Minor suggestion (non-blocking): The Settings and Extensions sections currently document only user and installation levels. Maven 4 also supports a project level (${maven.project.conf}/settings.xml and ${maven.project.conf}/extensions.xml per Constants.MAVEN_PROJECT_SETTINGS / Constants.MAVEN_PROJECT_EXTENSIONS). The Properties section already describes the three-tier model, so mentioning the project tier in Settings/Extensions would make the document internally consistent. But that's a separate improvement — this PR is a clear net positive as-is.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
gnodet
left a comment
There was a problem hiding this comment.
The new commits since our last approval are all correct improvements:
- Project-level config paths added for Settings (
${session.rootDirectory}/.mvn/settings.xml) and Extensions (${session.rootDirectory}/.mvn/extensions.xml) — verified againstConstants.java(lines 147, 171) andmaven-system.properties(maven.project.conf = ${session.rootDirectory}/.mvn). The path style is consistent with existing docs in the same file (e.g.,jvm.config,maven-user.properties). - Backtick formatting for
DefaultRequestCacheclass name — correct. - Oxford commas in three-item enumerations — appropriate for technical documentation.
Nice work addressing the three-tier configuration model. This now accurately documents all three levels (installation, user, project) for both Settings and Extensions.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
|
@elharo Please assign appropriate label to PR according to the type of change. |
Uh oh!
There was an error while loading. Please reload this page.