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
chore: remove deprecated method #95
Conversation
Codecov Report
@@ Coverage Diff @@
## master #95 +/- ##
=========================================
Coverage 77.32% 77.32%
Complexity 1108 1108
=========================================
Files 73 73
Lines 5915 5915
Branches 645 645
=========================================
Hits 4574 4574
Misses 1014 1014
Partials 327 327 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per semver removing a deprecated method requires a major version bump because it's backwards incompatible. I suspect that should be done in this PR.
@@ -50,14 +46,13 @@ public void testHashCode() { | |||
assertEquals(OPTION.hashCode(), OPTION_EQUALS.hashCode()); | |||
} | |||
|
|||
@Test | |||
@Test(expected = NullPointerException.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google best practice is not to use expected exceptions. As of JUnit 4.13 you can use assertThrows instead.
Codecov Report
@@ Coverage Diff @@
## master #95 +/- ##
============================================
- Coverage 77.32% 77.26% -0.06%
Complexity 1108 1108
============================================
Files 73 73
Lines 5915 5904 -11
Branches 645 635 -10
============================================
- Hits 4574 4562 -12
+ Misses 1014 1011 -3
- Partials 327 331 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't change the public API without a major version bump.
try { | ||
new Option(null, VALUE) {}; | ||
Assert.fail(); | ||
} catch (NullPointerException ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ex --> expected in this pattern, to avoid problems with static code analysis
new Option(null, VALUE) {}; | ||
Assert.fail(); | ||
} catch (NullPointerException ex) { | ||
assertNull(ex.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is current behavior, but there should be a message here and I'd prefer not to lock in its nonexistence with this assert.
Fixes #68