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

Added a comment for @SuppressFBWarnings("DM_EXIT") #623

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

sunidhi-prabhu
Copy link
Contributor

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable approach, though I think the more common approach with SuppressFBWarnings is to place the value and the justification as arguments to the SuppressFBWarnings annotation. Refer to the Javadoc documentation for more details.

@MarkEWaite MarkEWaite added the documentation Improvements or additions to documentation label Dec 8, 2023
@MarkEWaite
Copy link
Contributor

@sunidhi-prabhu I'll wait to merge this until you comment if you intend to update to use the more common form of justification for a spotbugs warning suppression.

@sunidhi-prabhu
Copy link
Contributor Author

Indeed, the intention is to enhance clarity by including both the warning code and justification. However, upon reviewing the @SuppressFBWarnings documentation, it appears that there is no dedicated section for justification in the code, and only the 'value' parameter is specified. In consideration of this, I opted for a precise approach, aligning with the tool's capabilities to avoid potential confusion for future developers. The goal remains to maintain code clarity and adherence to established conventions within the tool's framework.
Your insights on this matter would be highly valuable @MarkEWaite

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Dec 9, 2023

However, upon reviewing the @SuppressFBWarnings documentation, it appears that there is no dedicated section for justification in the code, and only the 'value' parameter is specified.

We use justification in many of the spotbugs suppressions in Jenkins. Here are some examples for your reference:

A more exhaustive list is available with a GitHub search.

@sunidhi-prabhu
Copy link
Contributor Author

Thanks for your feedback, I've made the changes and have included a more detailed justification for the usage of SuppressFBWarnings.

Let me know if everything looks good from your end, or if there's anything else you'd like me to address.

@MarkEWaite
Copy link
Contributor

No further changes requested from me. I see from the change that you decided to stay with the atypical pattern of placing the comment rather than assigning to the justification argument as is done in the other examples in the Jenkins project. That surprises me, but it is not a blocking concern.

Merging.

@MarkEWaite MarkEWaite merged commit df7a71f into jenkinsci:master Dec 11, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants