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

Fix the weird messages in DontReusePublicIdentifiers #2689

Merged
merged 9 commits into from
Nov 24, 2023

Conversation

JuditKnoll
Copy link
Collaborator

This PR fixes #2646, the problem with the messages in the PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS bugs.
With this change, the annotations of the BugInstances in the DontReusePublicIdentifiers detector match the messages.
Tried it out locally by printing out the messages, and all of them looked okay. I haven't committed this, since IMO it wouldn't be nice to check the hardcoded exact messages in the tests. If anyone has a good idea, how this could be properly tested, I'm open to any idea.
Also, I found PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_INNER_CLASS_NAMES bugpattern in messages.xml, which seems to be only existing in a stage of the development of #2511. (The other references to it are already deleted, both in the code and in findbugs.xml.)

Co-authored-by: @norbo03


Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

@hazendaz hazendaz self-assigned this Nov 9, 2023
@hazendaz
Copy link
Member

hazendaz commented Nov 9, 2023

@JuditKnoll Looks ok to me, are you saying you want to hold off on this for now? Maybe place in draft or was this one ok to merge? Seems we are going to be releasing again very shortly :(

@gtoison
Copy link
Contributor

gtoison commented Nov 9, 2023

I think the missing piece is finding a better way to catch the case when some annotations are missing. But detectors create them programmatically in non always trivial ways, so it is not easily checked.
In any case detecting such errors would be better suited for another PR, that one fixes an outstanding error

@norbo03
Copy link
Contributor

norbo03 commented Nov 9, 2023

@JuditKnoll The modifications look great to me.

@JuditKnoll
Copy link
Collaborator Author

I think it's okay to merge, since it solves the underlying problem. I was just wondering how it could be properly tested and showed that it works. If you think, there is no need to add test to this PR, then it can be merged.

@JuditKnoll JuditKnoll mentioned this pull request Nov 13, 2023
1 task
@JuditKnoll
Copy link
Collaborator Author

I think the missing piece is finding a better way to catch the case when some annotations are missing. But detectors create them programmatically in non always trivial ways, so it is not easily checked. In any case detecting such errors would be better suited for another PR, that one fixes an outstanding error

Created an issue from this comment, so it's doesn't get forgotten so easily.

@hazendaz You assigned this issue to yourself. Does it mean that you would like to merge it? I think it can be merged, if nobody finds any problem with it. I think it would be great, if this PR could be merged, before releasing 4.8.2.

@hazendaz hazendaz merged commit de4e3fe into spotbugs:master Nov 24, 2023
6 checks passed
@hazendaz
Copy link
Member

@JuditKnoll Sorry about that, I was probably thinking it would be merged around time I added myself there and simply didn't remove myself after it appeared it might sit a bit longer. If you see that elsewhere, don't think that means I mean to merge. Feel free to do so where needed.

@JuditKnoll
Copy link
Collaborator Author

@JuditKnoll Sorry about that, I was probably thinking it would be merged around time I added myself there and simply didn't remove myself after it appeared it might sit a bit longer. If you see that elsewhere, don't think that means I mean to merge. Feel free to do so where needed.

No problem. Thanks. Next time I will do so 😄

@JuditKnoll JuditKnoll deleted the DCL01-J-weird-messages branch November 24, 2023 08:11
@hazendaz hazendaz added this to the SpotBugs 4.8.2 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_CLASS_NAMES message missing substitutions
5 participants