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

Take changes to annotations into account #385

Closed
guillermocalvo opened this issue Feb 25, 2024 · 4 comments · Fixed by #396
Closed

Take changes to annotations into account #385

guillermocalvo opened this issue Feb 25, 2024 · 4 comments · Fixed by #396

Comments

@guillermocalvo
Copy link
Collaborator

Currently, the only annotation change that is reported is the addition of the @Deprecated annotation.

While adding or deleting annotations does not break compatibility with pre-existing binaries, changes in other types of annotations can also have a semantic impact in terms of backward compatibility. For example, adding or removing jakarta.transaction.Transactional to a method may affect client code invoking that method.

In any case, it would be a nice improvement if modifications to annotations were reported as PATCH changes.

@siom79
Copy link
Owner

siom79 commented Apr 22, 2024

Released with 0.21.0.

@siom79 siom79 closed this as completed Apr 22, 2024
@siom79
Copy link
Owner

siom79 commented Apr 23, 2024

The change did cause issue #395.

For me the implementation of calling checkIfMethodsHaveChangedIncompatible() and checkIfFieldsHaveChangedIncompatible() in this context seems to be wrong, as these methods are supposed to be run on the methods and fields of a class (and an annotation is a class itself). Hence; these methods are anyhow called on an annotation, when the annotation itself is processed as "class".

What we want to track here are the changes of the values of an annotation, not the changes of the annotation itself. If a method is removed from an annotation in a new version, then the value of this method must also be removed.

Therewith we also don't need ANNOTATION_MODIFIED_INCOMPATIBLE, as the annotation class itself will already be changed incompatible, but not its usage as annotation.

@guillermocalvo
Copy link
Collaborator Author

@siom79 Sorry about the trouble my PR caused. I believe your latest changes should fix #395.

I agree that ANNOTATION_MODIFIED_INCOMPATIBLE is not really needed; especially since you introduced the compatibility change ANNOTATION_MODIFIED which checks annotation elements.

Therefore, I think this issue can be closed now, because the functionality is complete. However, I noticed that ANNOTATION_MODIFIED is not currently being tested, so I prepared a small PR to hopefully expand test coverage. Thanks!

@siom79
Copy link
Owner

siom79 commented Apr 24, 2024

@guillermocalvo No problem, this can happen. While merging the PR I also haven't realized that annotating an annotation's element with the annotation itself will cause an endless recursion.

Thanks for the PR.

Version 0.21.1 is on its way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants