Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compatibilityChangeExcludes configuration #55

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

simonbasle
Copy link
Contributor

This commit adds the compatibilityChangeExcludes configuration option
which allows to simply turn well-known JApiCompatibilityChange enum
values into compatible changes, effectively excluding all instances
of such violations from the API comparison.

Fixes #43.

This commit adds the compatibilityChangeExcludes configuration option
which allows to simply turn well-known `JApiCompatibilityChange` enum
values into compatible changes, effectively excluding all instances
of such violations from the API comparison.

Fixes melix#43.
@melix
Copy link
Owner

melix commented Mar 29, 2022

Thanks for the PR! I think I would prefer, rather than excluding the problems, to actually turn them into accepted changes, so that they show up in reports. Basically, that would mean using the ViolationTransformer rule. WDYT?

@simonbasle
Copy link
Contributor Author

I have tried doing that, and it doesn't work well for the METHOD_NEW_DEFAULT case at least. I was running into blocker after blocker and found myself having to write a ton of logic, eg. to detect that a class-level violation ("class has incompatible changes" IIRC) with only accepted changes below (the METHOD_NEW_DEFAULT ones) should also be accepted / ignored, etc...

The library has a japicmp.cmp.JarArchiveComparatorOptions.OverrideCompatibilityChange and supports this use case (including in the Maven plugin), and I think that it is a valuable feature and a valuable convenience compared to ViolationTransformer (essentially supporting the METHOD_NEW_DEFAULT case with a configuration one-liner).

Sure this PR is opinionated compared to the Maven plugin, which allows to individually change the isBinaryCompatible() and isSourceCompatible() for each enum value. I refrained from doing that to avoid having to expose an equivalent of OverrideCompatibilityChange to plugin users for now.

Sure, with some elbow grease you could do the same using ViolationTransformers, but I found that's a LOT of elbow grease. And in my case, I gave up and painstakingly listed all the new default methods as methodExcludes instead.

@melix
Copy link
Owner

melix commented Mar 31, 2022

Right, indeed it's painful to figure out you need to silence the top level error when all children have been resolved. This is something I'd like to implement directly in the plugin.

@melix
Copy link
Owner

melix commented Sep 8, 2022

Given that I didn't have the opportunity to work on a "better" solution, I'll get this into the next release, thank you!

@melix melix self-assigned this Sep 8, 2022
@melix melix added this to the 0.4.1 milestone Sep 8, 2022
@melix melix merged commit 5172626 into melix:master Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for overrideCompatibilityChangeParameters
2 participants