-
Notifications
You must be signed in to change notification settings - Fork 165
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
(#1182) Remove some more static call to assert #1209
Conversation
Job #1209 is now in scope, role is |
This pull request #1209 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 |
@fabriciofx ping |
@fabriciofx please go on |
@paulodamaso can you refuse it for @fabriciofx so that someone else is assigned? |
@0crat assign me |
@paulodamaso There is an unrecoverable failure on my side. Please, submit it here:
0.50.34: CID: c6ab51d4-04b2-46b3-b2f9-b1568467eab9, Type: "Order was canceled" |
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.
@victornoel Why change the message for all assertions? Also,there is one comment, please take a look
new IsEqual<>("") | ||
); | ||
new Assertion<>( | ||
"must read empty string", |
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.
@victornoel I think that would be better to create a constant with the ""
(empty string), WDYT?
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.
@paulodamaso you mean a variable in the test for the empty string? I can do that yes
@paulodamaso This pull request #1209 is assigned to @paulodamaso/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 |
Manual assignment of issues is discouraged, see §19: -5 point(s) just awarded to @paulodamaso/z |
@paulodamaso thank you. I changed the messages because it seems it is the accepted way of describing an assertion with a positive sentence starting with "must". It has been asked to me many times in previous PRs. |
@victornoel Could you please change these messages back? As I understand, this message would be as like the error message for the assertion failure. This behavior is consistent with many other EO projects, so I think that we should stick to the standard |
@paulodamaso that's my point: I understood the standard in EO projects was to use a positive sentence starting with must. |
@victornoel It seems that we have both cases in |
35feebb
to
de831b8
Compare
@paulodamaso sorry for the delay, it is done and rebased on master |
Codecov Report
@@ Coverage Diff @@
## master #1209 +/- ##
============================================
- Coverage 89.22% 89.17% -0.06%
+ Complexity 1660 1659 -1
============================================
Files 279 279
Lines 3992 3992
Branches 213 213
============================================
- Hits 3562 3560 -2
- Misses 396 398 +2
Partials 34 34
Continue to review full report at Codecov.
|
@victornoel thanks for the fixes |
@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 12min) |
@0crat status |
@paulodamaso This is what I know about this job in C63314D6Z, as in §32:
|
@0crat quality acceptable |
@paulodamaso There is no quality review for #1209, no performer |
Code review was too long (10 days), architects (@paulodamaso) were penalized, see §55 |
The job #1209 is now out of scope |
Order was finished: +15 point(s) just awarded to @paulodamaso/z |
Payment to |
This is for #1182
I had to increment cactoos-matchers version to benefit from bugfixes (llorllale/cactoos-matchers#132) that were preventing tests to pass with
Assertion
.