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 NPE when calling ParsingException constructors with null reason #839

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

SirYwell
Copy link
Contributor

@SirYwell SirYwell commented Dec 7, 2022

A per the Javadocs, null values are permitted for the reason, however passing null or using a constructor that doesn't take a String causes a NullPointerException.

I addressed this issue and created a regression test for it.

Note: String#isBlank returns true if the string is empty, so this check wasn't needed.

@tsaglam tsaglam added bug Issue/PR that involves a bug minor Minor issue/feature/contribution/change labels Dec 7, 2022
@tsaglam tsaglam changed the base branch from main to develop December 7, 2022 11:26
Copy link
Member

@tsaglam tsaglam left a comment

Choose a reason for hiding this comment

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

Your branch is based on main but should be based on develop.

@SirYwell
Copy link
Contributor Author

SirYwell commented Dec 7, 2022

From my understanding of the comment in the pull request template https://github.com/jplag/JPlag/blob/main/.github/pull_request_template.md?plain=1#L3 together with semver (https://semver.org/#summary)

PATCH version when you make backwards compatible bug fixes

I thought main would be the appropriate branch. I can change the branch, but I think the template is confusing then.

@JanWittler JanWittler changed the base branch from develop to main December 7, 2022 12:26
@JanWittler
Copy link
Contributor

That's true, but your PR was targeting the develop branch. I adjusted it.

Copy link
Contributor

@JanWittler JanWittler left a comment

Choose a reason for hiding this comment

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

Just one smaller comment, besides that looks fine

@tsaglam
Copy link
Member

tsaglam commented Dec 7, 2022

I would still merge it into develop, as I am probably not releasing a patch version for this alone (I changed the target initially to develop).

@SirYwell SirYwell changed the base branch from main to develop December 7, 2022 13:23
@SirYwell
Copy link
Contributor Author

SirYwell commented Dec 7, 2022

I would still merge it into develop, as I am probably not releasing a patch version for this alone (I changed the target initially to develop).

I changed the target branch and rebased the commit onto develop.

@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@tsaglam tsaglam merged commit ab1a3d1 into jplag:develop Dec 12, 2022
@SirYwell SirYwell deleted the fix/ParsingException branch December 12, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue/PR that involves a bug minor Minor issue/feature/contribution/change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants