-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Delete unused mode constants #2022
Conversation
some unrelated flakage in CI: Error: Failed to read artifact descriptor for org.sonatype.sisu:sisu-guice:jar:no_aop:3.1.0: Failed to collect dependencies at org.apache.maven.plugins:maven-dependency-plugin:jar:3.3.0 -> org.apache.maven.shared:maven-common-artifact-filters:jar:3.2.0 -> org.eclipse.sisu:org.eclipse.sisu.plexus:jar:0.0.0.M5 -> org.sonatype.sisu:sisu-guice:jar:no_aop:3.1.0: The following artifacts could not be resolved: org.sonatype.sisu:sisu-guice:pom:3.1.0 (absent): Could not transfer artifact org.sonatype.sisu:sisu-guice:pom:3.1.0 from/to central (https://repo.maven.apache.org/maven2): HTTP connect timed out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the default mode, so obviously not usually explicitly defined, though I could find a couple of code that does use it:
https://github.com/search?q=combine.self%3D%22merge%22+&type=code
Removing the default value would not allow specifying the default behaviour anymore, which may break some use cases (not 100% sure).
Also, this removes the only place where those values are documented and leave the code with no documentation at all.
In order to remove the fact they are not used, maybe the merge
method could check if the attributes have known values (i.e. override
or merge
) and log something if that's not the case ?
But overall, I'm not sure what we are the benefits here, and I do see drawbacks.
This doesn't remove the mode. It removes the unused constant. If the constant is unused, should it be used somewhere? |
Right.
Maybe the merge method could check if the attributes have known values (i.e. |
Or throw an exception? |
Throwing an exception would indeed be easier. We'd need to assess that not too many POMs using invalid values... |
Superseded by #2038 |
We can always add these later if we need them, but right now their existence implies things about the code that aren't true.