-
Notifications
You must be signed in to change notification settings - Fork 181
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
[MDEP-964] Allowlist org.glassfish:javax.json #459
base: master
Are you sure you want to change the base?
Conversation
@@ -231,7 +231,7 @@ public abstract class AbstractAnalyzeMojo extends AbstractMojo { | |||
* | |||
* @since 2.10 | |||
*/ | |||
@Parameter(defaultValue = "org.slf4j:slf4j-simple::") | |||
@Parameter(defaultValue = "org.slf4j:slf4j-simple::,org.glassfish:javax.json::") |
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.
How big could this list of hidden
default exclude be? and why this javax.json specially?
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.
Probably less than 100 over time. Each one is an improvement.
javax.json because it's the next one I happened to stumble over when working on our own code.
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.
We should put it in other list ... when we use it as default it will be overridden when user use own list.
Should be added always, maybe we need a new parameter like useDefaultExcludes
or some else ...
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.
I'm not sure if we need add every dependencies which we need in a specific projects ....
In such way we will have very long list
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.
I don't think we will have a very long list. ChatGPT-4 has roughly 1.8 trillion parameters. In 2025 that's a long list. Here, there might be a few dozen special cases occasionally encountered, though unless someone spends a lot of time on this, we will probably include less than ten. This is a small list.
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.
nevermind how list is long will be lost when user add own excludes, so we should implement it in other way
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.
What do you suggest? We could include a base list of dependencies we always ignore and allow the user to add to this list.
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.
What do you suggest? We could include a base list of dependencies we always ignore and allow the user to add to this list.
exactly - user should have a possibility to not use default excluded list
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.
Do you want the user to replace the default list then, not simply append to it?
My gut is that we should not warn on anything we're not sure about, and we're never sure about dependencies like slf4j that are commonly used by reflection.
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.
Ping. There are two incompatible ways forward here and both have been requested by different people at different times. Either would be better than the status quo so we should choose one.
Option 1: There is a non-overridable list of dependencies we never warn about when they are declared but not used. A project can append to this list but not replace it.
Option 2: There is an overridable list of dependencies we never warn about when they are declared but not used. Projects can replace the entire list.
I prefer Option 1. It's simpler, and matches what almost everyone wants in practice. It emits fewer warnings and produces less noise. It will not break anyone's build. There are fewer false posiitives.
I'm OK with Option 2 if necessary. I think anything beyond these two options adds unnecessary complexity for no real world purpose.
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.
We should put it in other list ... when we use it as default it will be overridden when user use own list.
@@ -231,7 +231,7 @@ public abstract class AbstractAnalyzeMojo extends AbstractMojo { | |||
* | |||
* @since 2.10 | |||
*/ | |||
@Parameter(defaultValue = "org.slf4j:slf4j-simple::") | |||
@Parameter(defaultValue = "org.slf4j:slf4j-simple::,org.glassfish:javax.json::") |
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.
I'm not sure if we need add every dependencies which we need in a specific projects ....
In such way we will have very long list
No description provided.