brunoborges commented on PR #1599: URL: https://github.com/apache/maven-dependency-plugin/pull/1599#issuecomment-4178826869
## Feedback addressed & open questions for @rmannibucau Thanks for the thorough review! I've pushed changes addressing the actionable items (14 of 26 comments). The remaining items are design decisions I'd like your input on before proceeding. ### What was implemented 1. **`test-runtime` added** to `VALID_SCOPES` 2. **GAV format aligned** to `g:a[:ext[:cls]]:v` (matching `DefaultArtifact`). Scope removed from GAV — must be passed via `-Dscope`. 3. **Duplicate dependency now fails** instead of auto-updating (add ≠ update) 4. **In-memory model sync** after `PomEditor.save()` in both Add and Remove mojos 5. **NONE sentinel removed** — empty string is used instead 6. **`ByteArrayOutputStream`** for HTTP response reading 7. **No error message truncation** 8. **Single `log.info` calls** with `\n` instead of multiple calls 9. **Interactive TUI dropped entirely** (~400 lines of tests, ~200 lines of source removed) 10. **`PluginDescriptor`** for version detection (null-safe) 11. **Test assertion messages** include the actual string being checked ### Open questions — need your guidance I'd appreciate your thoughts on these design decisions before implementing: 1. **PomEditor approach** — You mentioned byte manipulation (L314) and also asked about Maven service (L104). For the first version, should we keep the current DOM approach and plan byte manipulation / Maven service for a follow-up, or is this a blocker? 2. **GAV vs split parameters** (L86) — You suggested picking one. Which do you prefer? I'm leaning toward keeping both since GAV is convenient for CLI use (`-Dgav=g:a:v`) while split params are better for POM-based plugin configuration. Happy to drop one if you feel strongly. 3. **`managed` as enum** (L119) — `YES/NO/BOTH` — is this about allowing the user to add a dependency to *both* `<dependencies>` and `<dependencyManagement>` in one invocation? 4. **`gav` as list** (L128) — For batch operations like `-Dgav=g1:a1:v1,g2:a2:v2`? Any preference on the delimiter? 5. **`bom` flag** (L135) — You said "not needed?" — should I remove it entirely and let users just set `-Dtype=pom -Dscope=import -Dmanaged=true` explicitly? 6. **Resolver API vs external search** (L67) — Fully agree this would be better. Is there a specific resolver API you have in mind for artifact search/discovery? The current Maven resolver APIs I'm aware of don't have a search/query capability equivalent to Central Search. 7. **johnzon-core for JSON parsing** (L496) — Happy to add this dependency. Should I replace all the manual JSON parsing with it? 8. **Rename `DependencyCoordinates`** (L40) — Since it holds scope/optional which aren't coordinates, any name suggestion? `DependencyDescriptor`? `DependencySpec`? 9. **Add version to `validate()`** (L98) — Should `validate()` require version to be non-null/non-empty, or just warn if missing? -- 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]
