Conversation
elharo
left a comment
There was a problem hiding this comment.
Also this is not a complete fix for Windows. Windows reserves a number of file names at the word -- not character level. If any of these showed up as a group ID or artifact ID or something else we try to create a file for in the local repo, Windows developers would have a bad day:
CON (Console)
PRN (Printer)
AUX (Auxiliary device)
NUL (Null device)
COM1 through COM9 (Serial communication ports)
LPT1 through LPT9 (Parallel printer ports)
Those constants are used to validate the |
gnodet
left a comment
There was a problem hiding this comment.
Clean readability improvement — the renamed constants (ILLEGAL_NTFS_FILENAME_CHARS, ILLEGAL_WINDOWS_PATH_CHARS) and their Javadoc make the Windows filesystem constraint explicit rather than implicit.
One observation: DefaultSettingsValidator.java has a duplicated ILLEGAL_REPO_ID_CHARS constant with a comment // ILLEGAL_FS_CHARS that still references the old name. The compat modules (compat/maven-model-builder/.../DefaultModelValidator.java and compat/maven-settings-builder/.../DefaultSettingsValidator.java) also have their own copies of these constants with the old names. Consider extending the rename to those files for consistency — though the compat modules are lower priority since they mirror the main implementation.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
I don't think this is an issue. It's only used to read the pom file relative to an existing pom.xml. And the goal of the previous PR was to reject known invalid paths before they cause an unwanted exception while constructing |
I feel there's a missing layer of indirection somewhere. Repository IDs and version strings should not be tightly coupled to the vagaries of one filesystem. We might want to untangle that. But for the moment this change at least makes the nature of the concern more explicit.