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

(#1182) Remove some more static call to assert #1209

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

victornoel
Copy link
Collaborator

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.

@0crat
Copy link
Collaborator

0crat commented Oct 12, 2019

Job #1209 is now in scope, role is REV

@0crat
Copy link
Collaborator

0crat commented Oct 12, 2019

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

@victornoel
Copy link
Collaborator Author

@fabriciofx ping

@paulodamaso
Copy link
Contributor

@fabriciofx please go on

@victornoel
Copy link
Collaborator Author

@paulodamaso can you refuse it for @fabriciofx so that someone else is assigned?

@paulodamaso
Copy link
Contributor

@0crat assign me

@0crat
Copy link
Collaborator

0crat commented Nov 1, 2019

@0crat assign me (here)

@paulodamaso There is an unrecoverable failure on my side. Please, submit it here:

PID: 12@b7e726625068, thread: PQ-C63314D6Z
com.zerocracy.claims.ClaimIn[214] java.lang.IllegalArgumentException: Parameter "age" not found in "Order was canceled" among: [comment, job, issue, repo, cause, _parent_sig, flow, login]

0.50.34: CID: c6ab51d4-04b2-46b3-b2f9-b1568467eab9, Type: "Order was canceled"

Copy link
Contributor

@paulodamaso paulodamaso left a 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",
Copy link
Contributor

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?

Copy link
Collaborator Author

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

@0crat
Copy link
Collaborator

0crat commented Nov 1, 2019

@0crat assign me (here)

@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

@0crat
Copy link
Collaborator

0crat commented Nov 1, 2019

Manual assignment of issues is discouraged, see §19: -5 point(s) just awarded to @paulodamaso/z

@victornoel
Copy link
Collaborator Author

@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.

@paulodamaso
Copy link
Contributor

@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

@victornoel
Copy link
Collaborator Author

@paulodamaso that's my point: I understood the standard in EO projects was to use a positive sentence starting with must.

@paulodamaso
Copy link
Contributor

@victornoel It seems that we have both cases in cactoos, so we have two standards. There is really no problem on this, since the messages fulfill their purpose of informing the assertion result. Taking this into account you can leave the messages as you reworded them, but please just capitalize the first word in each sentence then.

@victornoel
Copy link
Collaborator Author

@paulodamaso sorry for the delay, it is done and rebased on master

@codecov-io
Copy link

codecov-io commented Nov 2, 2019

Codecov Report

Merging #1209 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/io/ReaderOf.java 96.55% <0%> (-3.45%) 28% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e0da30...de831b8. Read the comment docs.

@paulodamaso
Copy link
Contributor

@victornoel thanks for the fixes

@paulodamaso
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Nov 6, 2019

@rultor merge

@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit de831b8 into yegor256:master Nov 6, 2019
@rultor
Copy link
Collaborator

rultor commented Nov 6, 2019

@rultor merge

@paulodamaso Done! FYI, the full log is here (took me 12min)

@paulodamaso
Copy link
Contributor

@0crat status

@0crat
Copy link
Collaborator

0crat commented Nov 6, 2019

@0crat status (here)

@paulodamaso This is what I know about this job in C63314D6Z, as in §32:

@paulodamaso
Copy link
Contributor

@0crat quality acceptable

@0crat
Copy link
Collaborator

0crat commented Nov 10, 2019

@0crat quality acceptable (here)

@paulodamaso There is no quality review for #1209, no performer

@victornoel victornoel deleted the 1182-no-more-static-assert branch November 10, 2019 09:09
@0crat 0crat removed the scope label Nov 12, 2019
@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

Code review was too long (10 days), architects (@paulodamaso) were penalized, see §55

@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

The job #1209 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

Order was finished: +15 point(s) just awarded to @paulodamaso/z

@0crat
Copy link
Collaborator

0crat commented Nov 12, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @paulodamaso/z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants