(#1335) Introduce NumberEnvelope and move number in their package #1587

victornoel · 2021-04-19T14:33:13Z

Last part of #1335: introduce NumberEnvelope and move all the Number implementations to their own packages.

Some tests were passing differently with respect to overflow and stuffs like that: I considered it was ok to change them, but I may have misunderstood their purpose. From my POV, they didn't bring anything particularly useful, but I will ask their original author in comments to review that.

This also covers #1591 and #1496 since they fitted nicely in this PR.

victornoel · 2021-04-19T14:35:16Z

src/test/java/org/cactoos/number/SumOfTest.java

@@ -125,7 +125,7 @@ void withIterableOfInts() {
    void overflowIntFromLongValues() {
        MatcherAssert.assertThat(
            new SumOf(2_147_483_647L + 1L << 1, 10L).intValue(),
-            new IsEqual<>(2_147_483_647)
+            new IsEqual<>(10)


@svendiedrichsen I think you originally wrote this test, and I wasn't sure exactly what it was supposed to verify or test, but after changing the way SumOf is implemented, now it gives this result. Do you think this is correct and if not, could you help me understand why? thx!

@victornoel I have initially written this test to assure that long overflows during summation don't affect the result. Original commit here: cb97bf0#diff-68e784b552b842bf92ef1a42843f896862c82e87df36dcd69d8cd2561b1ad3b4
SumOf used to do summation using double values and thus being prone to overflow.

@svendiedrichsen thank you for your answer. I think the commit you linked to did not change: I was referring to the test this comment here is attached to. I understand the general why, I had seen the commits and messages, but for this particular test, I don't exactly understand why the previous behaviour is correct, and why this new one would not. If you have some insight about it, it would be really helpful :)

@victornoel I can't remember anymore. Sorry. But if I had to guess. I think it is a test how the overflow is handled if you request an Int from a number larger than an int.

@svendiedrichsen ok, thanks :) I should find a satisfying solution I think, cheers

victornoel · 2021-05-02T10:33:49Z

@0crat status

0crat · 2021-05-02T11:14:46Z

@0crat status (here)

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

The job #1587 is in scope for 13days
The role is REV
The job is assigned to @andreoss /z for 13days
There is no monetary reward attached, it's a free job
The job doesn't have any impediments
The budget is 15 minutes/points
These users are banned and won't be assigned:
- @victornoel /z: This user reported the ticket
Job footprint (restricted area)

victornoel · 2021-05-08T16:49:16Z

@andreoss finally, this PR is ready to be reviewed. It's a bit big and I apologized for it, if it's too big, just tell me and I will have to break it into pieces, but if I can avoid it will be easier since there are both changes and moved classes :)

andreoss · 2021-05-08T22:23:58Z

src/main/java/org/cactoos/number/AvgOf.java

+ * @since 1.0.0
+ */
+@SuppressWarnings("serial")
+@SuppressFBWarnings("SE_NO_SERIALVERSIONID")


@victornoel Why not add seriaId?

@andreoss there are some implementation of number for which this do not make sense (the one based on 4 scalars mainly). IMHO EO and serializable classes does not make sense together.

That being said, your comment makes me think that maybe it's best to just leave serial id in case it makes sense :)

andreoss · 2021-05-08T22:26:21Z

src/main/java/org/cactoos/number/SumOf.java


 /**
- * Integer Scalar which sums up the values of other Scalars of the same type
+ * Int total of numbers.


@victornoel Bad renaming?

andreoss · 2021-05-08T22:27:00Z

src/test/java/org/cactoos/number/AvgOfTest.java

-            Matchers.equalTo(3)
+                1, 2, 3, 4
+            ),
+            new AllOf<Number>(new IsEqual<>(2.5), new IsEqual<>(2.5))


@victornoel Should it be twice? Why not use IsNumber
https://github.com/llorllale/cactoos-matchers/blob/master/src/main/java/org/llorllale/cactoos/matchers/IsNumber.java

victornoel · 2021-05-13T08:45:53Z

@andreoss thx, I took your comments into account

codecov-commenter · 2021-05-15T13:39:34Z

Codecov Report

Merging #1587 (510666e) into master (a9c3bdc) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1587      +/-   ##
============================================
- Coverage     90.16%   90.08%   -0.09%     
+ Complexity     1616     1603      -13     
============================================
  Files           302      298       -4     
  Lines          3784     3750      -34     
  Branches        121      122       +1     
============================================
- Hits           3412     3378      -34     
+ Misses          339      338       -1     
- Partials         33       34       +1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/cactoos/Fallback.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
src/main/java/org/cactoos/io/TailOf.java	`100.00% <ø> (ø)`	`10.00 <0.00> (ø)`
src/main/java/org/cactoos/iterable/IterableOf.java	`95.83% <100.00%> (-0.17%)`	`15.00 <1.00> (-1.00)`
src/main/java/org/cactoos/number/AvgOf.java	`100.00% <100.00%> (ø)`	`6.00 <6.00> (?)`
...main/java/org/cactoos/number/ComparableNumber.java	`100.00% <100.00%> (ø)`	`2.00 <2.00> (?)`
src/main/java/org/cactoos/number/DivisionOf.java	`100.00% <100.00%> (ø)`	`4.00 <4.00> (?)`
src/main/java/org/cactoos/number/MaxOf.java	`100.00% <100.00%> (ø)`	`6.00 <4.00> (?)`
src/main/java/org/cactoos/number/MinOf.java	`100.00% <100.00%> (ø)`	`6.00 <4.00> (?)`
...main/java/org/cactoos/number/MultiplicationOf.java	`100.00% <100.00%> (ø)`	`3.00 <2.00> (?)`
...c/main/java/org/cactoos/number/NumberEnvelope.java	`100.00% <100.00%> (ø)`	`10.00 <10.00> (?)`
... and 6 more

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 a9c3bdc...510666e. Read the comment docs.

…kage

victornoel · 2021-06-09T18:21:56Z

@0crat status

victornoel · 2021-06-10T05:52:21Z

@0crat status

0crat · 2021-06-10T06:11:48Z

@0crat status (here)

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

The job #1587 is in scope for 2mon
The role is REV
The job is assigned to @andreoss /z for 2mon
There is no monetary reward attached, it's a free job
The job doesn't have any impediments
The budget is 15 minutes/points
These users are banned and won't be assigned:
- @victornoel /z: This user reported the ticket
Job footprint (restricted area)

0crat · 2021-06-10T06:12:21Z

@0crat status (here)

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

The job #1587 is in scope for 2mon
The role is REV
The job is assigned to @andreoss /z for 2mon
There is no monetary reward attached, it's a free job
The job doesn't have any impediments
The budget is 15 minutes/points
These users are banned and won't be assigned:
- @victornoel /z: This user reported the ticket
Job footprint (restricted area)

victornoel · 2021-06-12T14:41:42Z

@rultor merge

rultor · 2021-06-12T14:42:17Z

@rultor merge

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

rultor · 2021-06-12T14:52:35Z

@rultor merge

@victornoel Done! FYI, the full log is here (took me 10min)

0crat · 2021-06-12T15:20:13Z

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

0crat · 2021-06-12T15:21:44Z

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

sereshqua · 2021-06-12T20:02:46Z

@0crat quality good

0crat added 0crat/scope 0crat/role/REV labels Apr 19, 2021

victornoel commented Apr 19, 2021

View reviewed changes

victornoel mentioned this pull request May 3, 2021

New matchers to compare easily a number with another one llorllale/cactoos-matchers#235

Open

victornoel force-pushed the 1335-next branch 3 times, most recently from fa754b6 to 5fd8103 Compare May 8, 2021 16:42

andreoss suggested changes May 8, 2021

View reviewed changes

victornoel force-pushed the 1335-next branch from 5fd8103 to 2057596 Compare May 13, 2021 08:48

victornoel mentioned this pull request May 14, 2021

Fix Github Checks #1578

Closed

victornoel force-pushed the 1335-next branch from 2057596 to eac0262 Compare May 15, 2021 13:39

victornoel force-pushed the 1335-next branch 2 times, most recently from 51955aa to 3cab66c Compare May 16, 2021 14:17

victornoel added 3 commits May 16, 2021 16:40

(yegor256#1335) Introduce NumberEnvelope and move number in their pac…

2f9435b

…kage

(yegor256#1591) MinOf/MaxOf rejects empty Iterable

7a66067

(yegor256#1335) Add todo to investigate overflow in SumOf

510666e

victornoel force-pushed the 1335-next branch from 3cab66c to 510666e Compare May 16, 2021 14:40

rultor merged commit 510666e into yegor256:master Jun 12, 2021

victornoel deleted the 1335-next branch June 12, 2021 14:58

0crat added the qa label Jun 12, 2021

0crat removed the 0crat/scope label Jun 12, 2021

0crat added quality/good and removed qa labels Jun 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#1335) Introduce NumberEnvelope and move number in their package #1587

(#1335) Introduce NumberEnvelope and move number in their package #1587

victornoel commented Apr 19, 2021 •

edited

Loading

victornoel Apr 19, 2021

svendiedrichsen Apr 19, 2021 •

edited

Loading

victornoel Apr 19, 2021

svendiedrichsen Apr 19, 2021 •

edited

Loading

victornoel Apr 21, 2021

victornoel commented May 2, 2021

0crat commented May 2, 2021

victornoel commented May 8, 2021

andreoss May 8, 2021

victornoel May 13, 2021

andreoss May 8, 2021

andreoss May 8, 2021

victornoel commented May 13, 2021

codecov-commenter commented May 15, 2021 •

edited

Loading

victornoel commented Jun 9, 2021

victornoel commented Jun 10, 2021

0crat commented Jun 10, 2021

0crat commented Jun 10, 2021

victornoel commented Jun 12, 2021

rultor commented Jun 12, 2021

rultor commented Jun 12, 2021

0crat commented Jun 12, 2021

0crat commented Jun 12, 2021

sereshqua commented Jun 12, 2021

(#1335) Introduce NumberEnvelope and move number in their package #1587

(#1335) Introduce NumberEnvelope and move number in their package #1587

Conversation

victornoel commented Apr 19, 2021 • edited Loading

victornoel Apr 19, 2021

Choose a reason for hiding this comment

svendiedrichsen Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

victornoel Apr 19, 2021

Choose a reason for hiding this comment

svendiedrichsen Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

victornoel Apr 21, 2021

Choose a reason for hiding this comment

victornoel commented May 2, 2021

0crat commented May 2, 2021

victornoel commented May 8, 2021

andreoss May 8, 2021

Choose a reason for hiding this comment

victornoel May 13, 2021

Choose a reason for hiding this comment

andreoss May 8, 2021

Choose a reason for hiding this comment

andreoss May 8, 2021

Choose a reason for hiding this comment

victornoel commented May 13, 2021

codecov-commenter commented May 15, 2021 • edited Loading

Codecov Report

victornoel commented Jun 9, 2021

victornoel commented Jun 10, 2021

0crat commented Jun 10, 2021

0crat commented Jun 10, 2021

victornoel commented Jun 12, 2021

rultor commented Jun 12, 2021

rultor commented Jun 12, 2021

0crat commented Jun 12, 2021

0crat commented Jun 12, 2021

sereshqua commented Jun 12, 2021

victornoel commented Apr 19, 2021 •

edited

Loading

svendiedrichsen Apr 19, 2021 •

edited

Loading

svendiedrichsen Apr 19, 2021 •

edited

Loading

codecov-commenter commented May 15, 2021 •

edited

Loading