Skip to content

Check for null result of getFixVersion#661

Closed
davedevretro wants to merge 4 commits intojenkinsci:masterfrom
davedevretro:658-GetFixVersions
Closed

Check for null result of getFixVersion#661
davedevretro wants to merge 4 commits intojenkinsci:masterfrom
davedevretro:658-GetFixVersions

Conversation

@davedevretro
Copy link
Contributor

@davedevretro davedevretro commented May 23, 2025

Issue #658

Check for null result of getFixVersions as per example on atlassian site:
https://developer.atlassian.com/server/jira/platform/creating-workflow-extensions/#step-2.-edit-the-code

Tested on my local Jenkins instance.
🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@davedevretro davedevretro requested a review from a team as a code owner May 23, 2025 07:37
@rantoniuk
Copy link
Member

Thanks for the contribution! I see you have checked Ensure you have provided tests but I don't see any test file modified, did you miss adding it to the PR?

@davedevretro
Copy link
Contributor Author

Sorry I thought this meant that I was confirming it was tested - unticked now.

Copy link
Member

@rantoniuk rantoniuk left a comment

Choose a reason for hiding this comment

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

Build fails because of format check failure.
You need to run mvn spotless:apply after all changes, before committing changes


Iterable<Version> versions = issue.getFixVersions();
if(versions != null) {
for (Version currentVersion : versions) {
Copy link
Member

Choose a reason for hiding this comment

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

How about using ListUtils.emptyIfNull() instead of adding the if clause?

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that, we're stuck on version 3 for now, that doesn't have this.

Copy link
Member

Choose a reason for hiding this comment

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

Found another way using Java8 Optional:

for (Version currentVersion : Optional.ofNullable(issue.getFixVersions()).orElse(Collections.emptyList())) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it starting to break readability for minimal gain at this point?

Copy link
Member

Choose a reason for hiding this comment

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

I think it looks cleaner than adding another nested if statement, especially when there is no else (maybe we should consider adding LOGGING.fine()) , but it's fine if you want to leave it as it is.

Would you be able to add a test for this? I think this should be pretty easy by mocking the Null response object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't work out how to get a unit test to work for this case - sorry.. :/

@davedevretro
Copy link
Contributor Author

updated formatting

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