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

#1376: Scalar - Replace all the IsEqual with ScalarHashValue #1383

Merged
merged 1 commit into from
May 11, 2020

Conversation

marceloamadeu
Copy link
Contributor

This PR is for #1376

@codecov-io
Copy link

codecov-io commented May 9, 2020

Codecov Report

Merging #1383 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1383   +/-   ##
=========================================
  Coverage     89.69%   89.69%           
  Complexity     1667     1667           
=========================================
  Files           275      275           
  Lines          3959     3959           
  Branches        211      211           
=========================================
  Hits           3551     3551           
  Misses          375      375           
  Partials         33       33           

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 b1c6e9a...aa3d431. Read the comment docs.

@marceloamadeu
Copy link
Contributor Author

#1367 was removed because I didn't find any more cases of IsEqual that should be exchanged for ScalarHashValue.

To confirm, can you tell if in this case it is possible to use ScalarHashValue?

    public void dividesIntNumbers() {
        new Assertion<>(
            "Must divide int numbers",
            new DivisionOf(4, 2).intValue(),
            new IsEqual<>(2)
        ).affirm();
    }

Tks

@0crat
Copy link
Collaborator

0crat commented May 9, 2020

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

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.

@marceloamadeu just one comment to ensure all is well in terms of todos :)

@@ -29,9 +29,6 @@
* Assertion from cactoos-matchers. Once there is no more usage of
* MatcherAssert.assertThat, add the signature of MatcherAssert.assertThat to
* forbidden-apis.txt
* @todo #1367:30min Replace all the IsEqual with ScalarHashValue in Scalar
Copy link
Collaborator

Choose a reason for hiding this comment

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

@marceloamadeu have you verified there is really no IsEqual incorrectly used in the tests of scalar package?

@victornoel
Copy link
Collaborator

@marceloamadeu indeed, it seems to be ok then, thanks

@paulodamaso looks good to me

@paulodamaso
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented May 11, 2020

@rultor merge

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

@rultor rultor merged commit aa3d431 into yegor256:master May 11, 2020
@rultor
Copy link
Collaborator

rultor commented May 11, 2020

@rultor merge

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

@0crat 0crat added qa and removed scope labels May 11, 2020
@0crat
Copy link
Collaborator

0crat commented May 11, 2020

@sereshqua/z please review this job completed by @victornoel/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

@sereshqua
Copy link

@victornoel please make sure you will find at least 3 issues during next CR, thanks

@victornoel
Copy link
Collaborator

@sereshqua yes ^^ it's quite hard with this kind of PR :)

@sereshqua
Copy link

@0crat quality acceptable

@marceloamadeu marceloamadeu deleted the 1376 branch May 14, 2020 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants