(#132) TextMatcherEnvelope should call asString() only once at all
#147
Conversation
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
=========================================
Coverage 97.68% 97.68%
Complexity 112 112
=========================================
Files 24 24
Lines 259 259
Branches 3 3
=========================================
Hits 253 253
Misses 6 6
Continue to review full report at Codecov.
|
|
Job #147 is now in scope, role is |
|
This pull request #147 is assigned to @fabriciofx/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/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 |
| ).affirm(); | ||
| } | ||
|
|
||
| private static final class TextEquals extends TextMatcherEnvelope { |
There was a problem hiding this comment.
@victornoel IMHO all classes should be public. Why? Because if they're private you're obstructing the reuse of this functionalities by others parts. @paulodamaso WDYT?
There was a problem hiding this comment.
@fabriciofx it's a class that is part of the test: it's not meant to be reused. If someone wants that functionality, there is TextIs, but here the point is to te be self-sufficient with the envelope to test its behaviour. If there was any reason to reuse this class, then it would be as part of this project, since it's in the test sources, and in that case, when it is needed, we can always bring it outside of this class and then test it before making it public. But I wouldn't recommend it because it would defeat the purpose of having a self-sufficient test.
The real problem here is that TextMatcherEnvelope is an abstract class with behaviour that needs to be tested, this is not great, see #135.
There was a problem hiding this comment.
@fabriciofx @victornoel For me it's okay the way it is now; if we need to use this in he future, we can extract it.
There was a problem hiding this comment.
@fabriciofx are we good to merge then?
There was a problem hiding this comment.
@victornoel ok. @paulodamaso can you merge it now, please?
|
@paulodamaso can you merge it, please? |
|
@fabriciofx @victornoel thanks |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@paulodamaso Done! FYI, the full log is here (took me 3min) |
|
The job #147 is now out of scope |
|
@sereshqua/z please review this job completed by @fabriciofx/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
Code review was too long (8 days), architects (@paulodamaso) were penalized, see §55 |
|
Payment to |
|
@fabriciofx please make sure you will try to find at least 3 issues during next CR, thanks |
|
@sereshqua ok |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @fabriciofx/z |
|
Quality review completed: +4 point(s) just awarded to @sereshqua/z |
This is for #132
Since there are no
Stickyimplementation forTextin cactoos, I simply simulated it by callingasString()in the matches method.The problem originally appeared because it wasn't possible to test streams with text matchers so I added a test that reflected this use case.
I'm personally not very excited about the fact that
TextEnvelopedoes so much and it is hard to test and honestly a bit brittle to do things like this, but #135 covers that matter. @paulodamaso I hope you will reconsider it since it's not in scope currently :)