Add per-resource encoding support (MRESOURCES-281) - #406

Per-resource encoding — encoding path not actually exercised by the test.

The implementation logic at the filtering level looks correct. However, the test has a significant gap that makes it a no-op for validating the feature.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after bd24ef7 — both previous findings addressed.

1. Test now exercises the encoding pipeline — filtering=true + useDefaultFilterWrappers=true ensures the read→decode→filter→encode→write path is taken. The ${project.name} placeholder resolution proves the encoding is actually propagated (would fail silently with the old filtering=false binary-copy path). Non-ASCII round-trip assertions (café) validate both UTF-8 and windows-1252 codepaths. Solid.

2. propertiesEncoding bypass comment — confirmed present since the initial commit (line 277). No action needed.

Implementation is clean: 4-line precedence check with clear fallback, complete Javadoc with @since 3.4.0, and a test that would actually catch regressions.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after 7 new commits (bd24ef7→30c97de) — ChangeDetection strategy + separate input/output encoding.

The ChangeDetection refactoring is well-structured and the encoding conversion support is solid. Two findings:

1. Missing ChangeDetection tests — the 5-branch switch in FilteringUtils.copyFile() has zero test coverage. TIMESTAMP, ALWAYS, NEVER, and TIMESTAMP_AND_CONTENT are never exercised. Only CONTENT is implicitly tested. This is a significant gap for a core file-copy strategy that directly controls build behavior.

2. Stale error message — the flatten-conflict error at DefaultMavenResourcesFiltering:270 still says "overwrite was not set to true" even though the overwrite field was replaced by ChangeDetection. Minor, but confusing for users.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after 5 new commits (dec4590..9663f00) — PR expanded from per-resource encoding to 3 features.

New features reviewed:

1. ChangeDetection strategy (MRESOURCES-397) — Clean enum with 5 strategies (TIMESTAMP, CONTENT, TIMESTAMP_AND_CONTENT, ALWAYS, NEVER). Switch statement covers all values with a defensive default. copyUnconditionally (direct write) vs copyIfContentsChanged (CachingOutputStream/CachingWriter) split is correct — unconditional always returns true, content-change path returns the actual modification status.

2. Separate input/output encoding (MRESOURCES-232) — Enables encoding conversion during filtering (e.g. read ISO-8859-1, write UTF-8). When no filter wrappers are applied, raw byte copy ignores encoding (documented). The getOutputEncoding() fallback chain (per-resource → global output → input encoding) is clean.

3. Previous findings (MRESOURCES-281) — Test now exercises the encoding pipeline with filtering=true, placeholder resolution (${project.name}), and non-ASCII round-trip assertions. Both findings from the initial review remain addressed.

Key checks:

• copyOf() copies all new fields (outputEncoding, outputPropertiesEncoding, changeDetection) ✓
• copyFilePermissions correctly skipped when needsCopy=false (NEVER with existing file) ✓
• Graceful binary handling properly layered: DefaultMavenResourcesFiltering catches from copyFileWithResult(gracefulBinaryHandling=false) and retries without filtering ✓
• isOverwrite()/setOverwrite() correctly deprecated as wrappers around ChangeDetection.ALWAYS/CONTENT ✓
• @since tags consistently use 4.0.0-beta-2 for ChangeDetection, 4.0.0-beta-3 for encoding features ✓

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after 5845747 — PR now bundles ChangeDetection strategy (MRESOURCES-397) + separate input/output encoding (MRESOURCES-232) on top of the original per-resource encoding (MRESOURCES-281).

Solid implementation across all three features. Key checks:

• ChangeDetection switch logic — all five modes (NEVER/ALWAYS/TIMESTAMP/CONTENT/TIMESTAMP_AND_CONTENT) correctly separate the needsCopy decision from the unconditionally vs copyIfContentsChanged dispatch. CachingOutputStream/CachingWriter are properly confined to the content-detection paths, while unconditional copies use direct Files.newOutputStream/Files.newBufferedWriter.
• Encoding conversion — inputEncoding/outputEncoding split flows correctly through DefaultMavenResourcesFiltering → MavenFileFilter.copyFileWithResult → FilteringUtils.copyFile. Per-resource overrides take precedence over global settings at every level.
• getOutputEncoding fallback chain — returns inputEncoding when no output encoding is configured (backward-compatible no-conversion default), respects outputPropertiesEncoding for .properties files.
• Backward compatibility — isOverwrite()/setOverwrite() deprecated and delegated to ChangeDetection.ALWAYS/CONTENT. overwrite field removed but API preserved. The old copyFile(... boolean overwrite) in FilteringUtils deprecated and forwarded correctly.
• gracefulBinaryHandling at two levels — DefaultMavenFileFilter.doCopyFile retains it for the old copyFile(... gracefulBinaryHandling) path, while copyFileWithResult passes false so the exception propagates to DefaultMavenResourcesFiltering.filterResources() which handles it at the resource level. No double-catch risk.
• copyOf() — all new fields (outputEncoding, outputPropertiesEncoding, changeDetection) correctly included.
• Tests — filterResourcesWithPerResourceEncoding (MRESOURCES-281), getOutputEncoding unit test, filterResourcesWithOutputEncoding + filterResourcesWithPerResourceOutputEncoding (MRESOURCES-232) all exercise the actual filtering pipeline with non-ASCII content and placeholder resolution.

No issues found.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Re-review after 3ac738c — both previous findings addressed.

1. Stale error message — fixed: now says "changeDetection is not set to ALWAYS" instead of "overwrite was not set to true".

2. ChangeDetection tests — 14 comprehensive tests added in FilteringUtilsTest covering all 5 strategies (ALWAYS, NEVER, TIMESTAMP, CONTENT, TIMESTAMP_AND_CONTENT). Each strategy tested with both existing and non-existing destination files, copied return value verified, file content asserted. Large-payload tests (40KB > 32KB CachingOutputStream buffer) correctly exercise the flush/isModified edge case for CONTENT and TIMESTAMP_AND_CONTENT.

No new issues found.

This review was generated by an AI agent, Hermès on behalf of @gnodet.