adjusted MavenMojoProjectParser#logParseErrors to also log poms which…

[nmck257]

… were parsed correctly but encountered an error in resolution

What's changed?

see subject

What's your motivation?

• POM Parse Errors are under-reported by Maven plugin rewrite#4353

Anything in particular you'd like reviewers to focus on?

In this implementation, these errors will get logged in the "Parsing source files..." step instead of the "Resolving poms..." step. Logging under the "resolving pom" step might be more intuitive, but this code change is more concise, and, gives an opportunity for the exclude-directory logic to run.

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

The message produced for ParseExceptionResult for a maven resolution issue is the entire source of the pom, with the relevant error markup comments. This is what will get logged here if --errors are enabled, and that's a bit bloated for log output. It might be nice to adjust that logic in rewrite-maven to instead produce an abbreviated summary of the problematic file's problems (like how git diff just skips to the changed lines, with a few lines of surrounding context)

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[nmck257]

side note, I'm thinking about a change like this in AbstractRewriteMojo, to elevate a little more error-reporting from recipes to the user if they add the --errors flag (without having to go all the way to -X debug output). haven't tested in detail yet

    protected ExecutionContext executionContext() {
        return new InMemoryExecutionContext(t -> {
            if (mavenSession.getRequest().isShowErrors()) {
                getLog().warn(t);
            } else {
                getLog().debug(t);
            }
        });
    }

[timtebeek]

Nice measured improvement here @nmck257 ! Appreciate how you connected the dots to spot that these were silently dropped; hope this avoids some confusion going forward.

[nmck257]

thanks for mirroring over to gradle! @timtebeek

[timtebeek]

I've taken a slightly different approach in this follow up, as we found that logging isn't working out great on a larger scale:
f592159...59404e5

Note the use of MojoFailureException, which still allows users to ignore partial failures if they really want to, and how we've pulled forward this feedback to where we parse the Maven pom files, as opposed to reporting these failures at the end.

[nmck257]

So the default behavior with this change will be to fail-fast the whole recipe execution (and print details) if we can't resolve pom(s)? I'm fine with that if we think that's the better choice.

Re ignoring partial failures -- does that mean that the user could instruct Maven to continue with other goals if the rewrite goal fails, or, something else?

[timtebeek]

I haven't tried it, but with --fail-never it looks like folks can ignore failures and continue with either the next task or next submodule; not sure which of those it'll end up being specifically, but figured that's at least a better option than the more definitive MojoExecutionException.

And yes indeed the thinking here is that a clear failure is better than skipping along and having changes not applied effectively, with a troublesome path to debugging.