#981 Text: Before and After #1014
Conversation
|
Job #1014 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #1014 +/- ##
============================================
+ Coverage 87.6% 87.65% +0.04%
- Complexity 1518 1524 +6
============================================
Files 265 267 +2
Lines 3889 3903 +14
Branches 213 215 +2
============================================
+ Hits 3407 3421 +14
Misses 433 433
Partials 49 49
Continue to review full report at Codecov.
|
|
This pull request #1014 is assigned to @victornoel/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
| */ | ||
| @SuppressWarnings({"PMD.CallSuperInConstructor", | ||
| "PMD.ConstructorOnlyInitializesOrCallOtherConstructors"}) | ||
| public After(final String text, final String boundary) { |
There was a problem hiding this comment.
@Iprogrammerr is the casting really needed?
There was a problem hiding this comment.
@victornoel Without that build will not pass. I do not why, I see nothing wrong with this constructor.
There was a problem hiding this comment.
@Iprogrammerr ok, I suppose it is because of yegor256/qulice#887 and PMD doesn't see the call to super. Let's leave it then
There was a problem hiding this comment.
@Iprogrammerr qulice 0.18.10 was released with a fix for this, can you see if upgrading qulice enables you to remove those suppresswarnings.
If it introduces too much work on other parts of the code, then add a @todo at the top of this class to upgrade qulice AND remove both the suppresswarnings in Before and in After please.
| @SuppressWarnings({"PMD.CallSuperInConstructor", | ||
| "PMD.ConstructorOnlyInitializesOrCallOtherConstructors"}) | ||
| public Before(final String text, final String boundary) { | ||
| super((Scalar<String>) () -> { |
There was a problem hiding this comment.
@Iprogrammerr is the casting really needed?
| */ | ||
| @Test | ||
| public void returnsEmptyIfThereIsNoBoundary() { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@Iprogrammerr please use cactoos-matchers Assertion
| */ | ||
| @Test | ||
| public void returnsEmptyIfStringIsBoundary() { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@Iprogrammerr please use cactoos-matchers Assertion
| */ | ||
| @Test | ||
| public void returnsAfterBoundaryString() { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@Iprogrammerr please use cactoos-matchers Assertion
| */ | ||
| @Test | ||
| public void returnsInputIfThereIsNoBoundary() { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@Iprogrammerr please use cactoos-matchers Assertion
| */ | ||
| @Test | ||
| public void returnsEmptyIfStringIsBoundary() { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@Iprogrammerr please use cactoos-matchers Assertion
| */ | ||
| @Test | ||
| public void returnsBeforeBoundaryString() { | ||
| MatcherAssert.assertThat( |
There was a problem hiding this comment.
@Iprogrammerr please use cactoos-matchers Assertion
There was a problem hiding this comment.
@victornoel Let's wait for #1023 then.
| * @param text Text representing the text value | ||
| * @param boundary String after which text will be split | ||
| */ | ||
| @SuppressWarnings({"PMD.CallSuperInConstructor", |
There was a problem hiding this comment.
@Iprogrammerr I'm surprised this is needed, can you confirm it is?
There was a problem hiding this comment.
@victornoel Yes, I was too. I do not know what qulice does not like there.
| * @param text Text representing the text value | ||
| * @param boundary String to which text will be split | ||
| */ | ||
| @SuppressWarnings({"PMD.CallSuperInConstructor", |
There was a problem hiding this comment.
@Iprogrammerr I'm surprised this is needed, can you confirm it is?
There was a problem hiding this comment.
@victornoel Same as above.
|
@Iprogrammerr ping :) |
|
@victornoel Warnings need to be suppressed as I have described. As far as replacing MatcherAssert we need to wait for #1023. |
|
@Iprogrammerr the PR to upgrade cactoos-matchers to 0.13 was merged (#1027 where you forgot to remove the puzzle, so the issue is still open…) |
|
@victornoel the issue for PR #1027, #1023, is not a puzzle. @Iprogrammerr should ask the reporter to close it. |
|
@llorllale ah yes, good point ^^ got lost in all those microtasks :P anyway, this means that @Iprogrammerr you are no longer blocked and can fix the comments in this PR (and the others blocked by #1023) |
|
@victornoel Tests are refactored. I have left a puzzle to update qulice, because this is how my attempt to do it ended: |
| @@ -31,6 +31,9 @@ | |||
| * <p>There is no thread-safety guarantee. | |||
| * | |||
| * @since 1.0 | |||
| * @todo #981:30min Update qulice version and remove | |||
There was a problem hiding this comment.
@Iprogrammerr please specify the minimum required version of qulice
There was a problem hiding this comment.
@victornoel Done.
|
@llorllale I think it's good to merge now |
|
@Iprogrammerr @victornoel I think I'm going to stick with this decision for now (delay upgrading qulice). I'm OK with leaving any suppressions so long as no dependencies are required for our production code. Therefore, @Iprogrammerr can you please remove that puzzle? |
|
@Iprogrammerr can we rename Let's not forget to rename the test files as well. |
|
@llorllale Done. Travis does not like commits from the past, same as #1026. |
|
@Iprogrammerr maybe you should rebase on master |
|
@victornoel No, it is the same. |
|
@Iprogrammerr if I look at all the commits of this PR (https://github.com/yegor256/cactoos/pull/1014/commits) I see many commits with incorrect formatting of the message. |
|
@victornoel Back then, this rule does not exist. |
@Iprogrammerr so what? the purpose of the rule is to test your whole PR so that it conforms to expected quality. It's the problem of the PR creator to ensure it is mergeable. |
|
@victornoel You are right, but something else is wrong. Look at #1049. Travis error is the same, but there is only one new commit. It stops at commit with message "(#1030) ComparableText is not equal to the same text (extends TestEnvelope)" which was merged a few days ago. |
|
@Iprogrammerr open a new issue to get this bug solved |
|
@victornoel You are right, here it is #1050. |
|
@Iprogrammerr can you please rebase on |
|
@llorllale Done. |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale Done! FYI, the full log is here (took me 14min) |
|
Code review was too long (17 days), architects (@llorllale) were penalized, see §55 |
|
The job #1014 is now out of scope |
|
Order was finished: +15 point(s) just awarded to @victornoel/z |
|
Payment to |
Two new classes with tests as described in #981 using new naming convention discussed in #913(ARC comment).