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

StashRepository: Use StringUtils.containsIgnoreCase() and StringUtils.isEmpty() #130

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

proski
Copy link

@proski proski commented Jul 27, 2019

*  StashRepository: Use StringUtils.containsIgnoreCase() and StringUtils.isEmpty()
   
   This improves readability of the code in isPhrasesContain(). It also
   fixes a low-priority FindBugs warning.

@jakub-bochenski
Copy link

triggers a FindBugs warning.

Where is that warning triggered? I see No errors/warnings found on the CI build.

@proski
Copy link
Author

proski commented Aug 5, 2019

It is only reported if the threshold set to low. Just like the Name member. It is also reported by the SpotBugs plugin in Eclipse.

@jakub-bochenski
Copy link

Thanks for the explanation.

I don't think it's a good idea to change the code to a more verbose version just to satisfy FB.

I couldn't find a full description of this issue (e.g on https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html), maybe I'm misunderstanding something. Could you maybe send me a link to full description?

@proski
Copy link
Author

proski commented Aug 5, 2019

Here's the description: http://findbugs.sourceforge.net/bugDescriptions.html#DM_CONVERT_CASE

I realize that some issues cannot be fixed without making the code harder to read. This PR is adding code, but just a little bit. The change is just making the code more explicit.

If you don't like it, I can close it.

I think we should switch to using SpotBugs, which is more actively maintained. That would give us a better picture of what issues need to be fixed.

….isEmpty()

This improves readability of the code in isPhrasesContain(). It also
fixes a low-priority FindBugs warning.
@proski proski changed the title StashRepository: Use explicit default locale for case-insensitive comparison StashRepository: Use StringUtils.containsIgnoreCase() and StringUtils.isEmpty() Aug 5, 2019
@proski
Copy link
Author

proski commented Aug 5, 2019

Rewritten using StringUtils. The code is actually more readable now.

SpotBugs also detects a low-priority warning in the original code. We'll be updated from FundBugs to SpotBugs when we update the parent POM to the latest version. It's recommended to keep the parent POM up to date, as it updates the development tools without updating the requirements. I plan to do it once #68 is merged. Otherwise I would need to add an exception for the code removed in that PR.

The parent POM recommends using Low SpotBugs threshold:

<!-- Defines a SpotBugs threshold. Use "Low" to discover low-priority bugs.
         Hint: SpotBugs considers some real NPE risks in Jenkins as low-priority issues, it is recommended to enable it in plugins.
      -->
    <spotbugs.threshold>${findbugs.threshold}</spotbugs.threshold>

@jakub-bochenski
Copy link

Agree that we should update parent pom and switch to spotbugs

@jakub-bochenski jakub-bochenski merged commit 2f1676d into jenkinsci:master Aug 6, 2019
@proski proski deleted the use-locale branch August 6, 2019 17:45
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.

2 participants