slachiewicz commented on PR #1677:
URL:
https://github.com/apache/maven-dependency-plugin/pull/1677#issuecomment-5234472447
### Copilot review: keeping `List<String>`
All four comments were about the same change — `remoteRepositories` going
from a comma-separated `String` to `List<String>` — so answering once here as
well as in the threads.
**Decision: the parameter stays `List<String>`.** It matches `list-classes`,
which has had this exact type since MDEP-924 in 3.7.0, and reverting would
leave the two sibling goals inconsistent for no gain.
The concern was worth checking rather than dismissing, so I tested it
against real Maven instead of reasoning about it. Both binding paths keep
working, and each is now pinned by an integration test:
| path | IT |
|---|---|
| `-DremoteRepositories=a,b` | `src/it/projects/get-remote-repositories` |
| `<remoteRepositories>a,b</remoteRepositories>` as element text |
`src/it/projects/get-remote-repositories-pom-config` |
The second is the real compatibility risk — it is how the parameter was
written while it was a `String`. Both ITs name two repositories in one
comma-separated value and exercise both accepted forms, `id::layout::url` and
`id::url`.
They are not vacuous. Collapsing the value into a single element,
`Collections.singletonList(String.join(",", remoteRepositories))`, fails the
goal with exactly the mode Copilot predicted:
```
Failed to execute goal ...:get (default-cli) on project
get-remote-repositories:
Invalid repository:
repo1::default::https://repo1.invalid/maven2,repo2::https://repo2.invalid/maven2
```
The mechanism: sisu-plexus' `AbstractCollectionConverter.csvToXml` turns a
plain string value into synthetic list children by splitting on `,`, and that
is the same code path for a user property and for POM element text.
Two of the comments were right about the *test* rather than the code:
`testRemoteRepositories` only ruled the failure out implicitly, since a
comma-bearing single element would split on `::` into five tokens and throw. It
now asserts the injected field directly. The documentation point was fair too —
the javadoc shows the one-per-element POM form alongside the comma form.
I did not add the suggested defensive comma-splitting inside the list: the
converter demonstrably splits already, so it would be unreachable for every
input a user can actually produce.
422 unit tests, 101 ITs, all green.
<sub>Drafted with Claude — please verify</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
