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

Removed nulls from Retry (#886) #1010

Merged
merged 3 commits into from
Feb 2, 2019
Merged

Removed nulls from Retry (#886) #1010

merged 3 commits into from
Feb 2, 2019

Conversation

BinaryIgor
Copy link
Contributor

@BinaryIgor BinaryIgor commented Jan 15, 2019

Solving #886 puzzle. According to recommendations by ARC (#918 (comment)), I simply removed the constructors using null and taking a Proc. Tests were modified as needed.

@0crat 0crat added the scope label Jan 15, 2019
@0crat
Copy link
Collaborator

0crat commented Jan 15, 2019

Job #1010 is now in scope, role is REV

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1010      +/-   ##
============================================
- Coverage     87.65%   87.58%   -0.08%     
+ Complexity     1524     1520       -4     
============================================
  Files           267      267              
  Lines          3903     3897       -6     
  Branches        215      215              
============================================
- Hits           3421     3413       -8     
- Misses          433      435       +2     
  Partials         49       49
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/cactoos/func/Retry.java 55% <ø> (-18.08%) 3 <0> (-4)
...rc/main/java/org/cactoos/func/IoCheckedBiProc.java 0% <ø> (ø) 0 <0> (ø) ⬇️

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 3abfb4c...2b0e538. Read the comment docs.

@0crat
Copy link
Collaborator

0crat commented Jan 15, 2019

This pull request #1010 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

@BinaryIgor
Copy link
Contributor Author

@victornoel Ping.

Copy link
Collaborator

@victornoel victornoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Iprogrammerr see my comments

@@ -35,6 +35,11 @@
* @param <X> Type of input
* @param <Y> Type of input
* @since 0.22
* @todo #861:30min Avoid usage of null value in exec(first, second),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Iprogrammerr the issue number should refer to the bug you are solving

},
count -> count == Integer.MAX_VALUE
).apply(true),
Matchers.nullValue()
Matchers.equalTo(true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Iprogrammerr please use FuncApplies instead of EqualTo and Assertion instead of assertThat from cactoos-matchers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victornoel We need to wait for #1023 to do that.

@BinaryIgor
Copy link
Contributor Author

@victornoel Ping.

@victornoel
Copy link
Collaborator

@Iprogrammerr thx
@llorllale good to merge

@victornoel
Copy link
Collaborator

@llorllale ping

@victornoel
Copy link
Collaborator

@llorllale still good to merge ;)

@llorllale
Copy link
Contributor

@Iprogrammerr can you please rebase on master?

@BinaryIgor
Copy link
Contributor Author

BinaryIgor commented Feb 2, 2019

@llorllale Done.

@llorllale
Copy link
Contributor

@Iprogrammerr I get the feeling that 0pdd doesn't like rebases or force-pushing (see yegor256/0pdd#99), so I think it may not automatically close #886 in this case. Let's see.

@llorllale
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 2, 2019

@rultor merge

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

@rultor rultor merged commit 2b0e538 into yegor256:master Feb 2, 2019
@rultor
Copy link
Collaborator

rultor commented Feb 2, 2019

@rultor merge

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

@0crat 0crat removed the scope label Feb 2, 2019
@0crat
Copy link
Collaborator

0crat commented Feb 2, 2019

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

@0crat
Copy link
Collaborator

0crat commented Feb 2, 2019

The job #1010 is now out of scope

@0crat
Copy link
Collaborator

0crat commented Feb 2, 2019

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

@0crat
Copy link
Collaborator

0crat commented Feb 2, 2019

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @llorllale/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