-
Notifications
You must be signed in to change notification settings - Fork 126
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
Test uniqueness of problem IDs #1754
Test uniqueness of problem IDs #1754
Conversation
@srikanth-sankaran @jarthana @iloveeclipse FYI. In particular let me know if you see problems with:
|
@@ -829,25 +829,35 @@ public interface IProblem { | |||
int AbstractMethodsInConcreteClass = TypeRelated + 333; | |||
|
|||
/** @deprecated - problem is no longer generated, use {@link #UndefinedType} instead */ | |||
@Deprecated |
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.
when those constants are not used anymore, can you please add "forRemoval"?
like
@Deprecated(forRemoval = true, since = "3.37.0 2024-03")
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.
The deprecation (in javadoc) is actually much older (older than the annotation type? likely older than "forRemoval" :) ) - should we still use the current version in since?
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.
the since refers to the forRemoval
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.
the since refers to the forRemoval
No,
String since
Returns the version in which the annotated element became deprecated.
And
boolean forRemoval
Indicates whether the annotated element is subject to removal in a future version.
There is no direct connection between both properties.
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.
@jukzi once more I scanned the constants in question, and found that those have been deprecated some time in the range 2005 - 2011. So proper marking would require to look up the exact JDT release for each of those constants to provide a proper since
value. Setting forRemoval
is a separate strategic decision altogether.
In this PR I only want to make the existing deprecations (javadoc) visible during a test (using the annotation). Can't we think of this as orthogonal to the issue of improving the details of deprecation? This PR should serve only one purpose: avoid ambiguous IDs of "active" problems, ignoring those that are no longer raised.
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.
My desire is to actually have unused code to be removed in the future. That should only be done 2 years after announcing the removal. For that purpose it does not matter when exactly deprecation happened, but it should have the forRemoval flag 2 years in front. If you do not feel confident about adding a rough date it is fine if you just ignore my request to add a date (or the forRemoval at all) and leave that burden for someone else in the future. In doubt we just live with the unused constants forever. Wont be of any problem.
Still the easiest and widely accepted solution (yes, slightly dirty, but most helpful) would be to temporarily just use the current's release version/date and delete it after 2 years.
- exclude deprecated constants (from pre Java 5 time / 2004) - mark deprecation using annotation (was not possible in 2004)
86641f1
to
c307e38
Compare
What it does
Whenever we add a new constant to
IProblem
it's getting harder by the day, to find a definitely unused ID (since new IDs are allocated in different ranges, and constants are not strictly sorted).So rather than breaking clients by renumbering, I simply wrote a reflective test that ensures that any new constant doesn't share the ID with an existing one.
This was first requested in https://bugs.eclipse.org/bugs/show_bug.cgi?id=509643.
Initially, my test did detect some existing duplicates, where it turned out, that one in each pair was
@deprecated
(in javadoc). In order to make this visible via reflection, I'm also adding the@Deprecated
annotation (which was not available in JDT/Core at the time of deprecating pre-1.5 problems).How to test
Add a duplicate to see the test fail :)