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
[core] Introduce a new @DeprecatedUntil annotation #4931
base: master
Are you sure you want to change the base?
Conversation
- This should be a better and more stable solution to the old @DeprecatedUntil700 we used during PMD 7's development. - It is not tied to a specific major. - It allows us to give more than 1 major to remove something if we ever want to… - It allows us to explicitly document what we want to do in the next major. - It doesn't force us to immediately apply other annotations such as @internalapi which lead to inconsistency (ie: is is already private or will be made private in the next major?)
Generated by 🚫 Danger |
Moving the discussion from #4926 (comment) to continue:
You can apply One thing we could do however is: Remove the annotations from the japicmp configuration and force us to explicitly list the members/classes that are internal in the excludes of the japicmp (you can also exclude individual members). We would then use XML comments in pom.xml to explain, what is internal api and why this or that is excluded etc.
Instead of creating another own annotation, we could also use https://github.com/apiguardian-team/apiguardian : the single annotation API might be all we need. I'm not sure about tooling support. I guess, with japicmp we would then need list the excluded members explicitly, as you can't just exclude If we introduced apiguardian now, we would still need to use the old annotations Whatever we do, we need to update https://github.com/pmd/pmd/blob/master/docs/pages/pmd/projectdocs/decisions/adr-3.md . Overall, I'm in favor of better API documentation. I'm not sure, whether we need to distinguish for deprecated members whether they will be removed or internalized - in any case, you shouldn't use it anymore. My idealistic understanding of deprecation is, that it will be gone with the next major version - so IMHO we don't need repeat |
The distinction is useful for us maintainers though, to know what to do when cleaning up before a major release. Maybe we should use a new annotation
The point is that between majors, we only use I also agree with Andreas here that we don't need to mention the major in our documentation annotations. I would like to keep the simple model that API is always deprecated until the next major, whichever that is. |
Why can't it be that simple that we simply delete all deprecations for a major release? I would strongly advice against introducing something like |
Maybe it's that simple, but I don't know. I'm not sure whether we will run into the same problems during the PMD 7 release cycle as during the PMD 6 one. For instance there were many classes which we wanted to hide in PMD 7 but not delete from the codebase. In that case I think in many cases it doesn't make sense to copy the class into an internal package and deprecate the old one. It might be plain impossible because it would introduce new types that are not necessarily compatible. Like I said I don't know if we will run into this scenario often, but we might chance into it at least eg to internalize XPathRule.
This is ideal but it is not always possible... I think there is still a distinction to be made between 'we want to hide this' and 'we want to delete this', but maybe just a comment is enough, coupled with tracking in an issue. And of course first try to do what you say, that is, refactor as soon as possible. |
Exactly, there will be scenarios where internalizing (hiding) is not compatible with adding a new method and deprecating the old one. For instance, wanting to reduce the visibility of a field / method from If it were a public non-final field (ugly, I know) we can't duplicate it, as we can't ensure to keep both attributes in sync. Yes, the vast majority of cleanups are effectively going to be deletes (and it has been so in PMD 6 and PMD 7), but I don't think it can ever be reduced to solely that without hindering or limiting the changes we can do. As for using API Guardian, I see 2 issues with it:
I'd rather not have to manually add each class and member to the pom… we have seen in PMD 7 development that list can be massive, and there is a strong chance we miss some. I think we will need this annotation, along with the current uses of I'm open to drop the |
I also don't know. But let's be optimistic. Let's not try to solve a problem, that we don't have yet. Let's not already give up trying to keep it simple without even having it tried at least once.
Honestly, this wasn't the case. We kept using the deprecated stuff all over the place. In this regards we must become better - only deprecate stuff once we refactored. Don't deprecate with a note: "PMD 8 will provide an alternative implementation".
I don't think, we will have the problem to miss something: We don't configure the includes - that means - japicmp will consider everything as public API. We only configure excludes. And if we miss something to exclude (e.g. some method we created as internal API and we change that method) then japicmp will complain. So, I don't see the problem that we might miss entries there. Does the list become too big? I don't know - I can't predict the future. If we consequently keep internal API in internal packages, the list won't grow so much. If the list becomes too big - maybe that's even an indication of another problem (not separating API and internal impl enough?) or it's time for some cleanup (what ever this means - as I said, I'm making stuff up now)
For reference, the only occurrence of a deprecation in PMD 7 is right now this: pmd/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/design/SingularFieldRule.java Lines 103 to 104 in 7da809b
I fully agree that we need something (annotation, tool, documentation) to better steer the API evolution/maintenance. However, right now, we are talking completely in theory, don't we? I would suggest to wait with a decision until we have a very concrete problem to solve. Then we can take this problem and maybe try so solve it in different ways and compare the results. I find it hard to solve right now a problem, that I need to make up at the moment. |
Describe the PR
Related issues
Ready?
./mvnw clean verify
passes (checked automatically by github actions)