#735 CheckedScalar #738

Vatavuk · 2018-03-13T21:50:05Z

See #735 CheckedScalar introduced

codecov-io · 2018-03-13T21:55:01Z

Codecov Report

Merging #738 into master will increase coverage by 5.39%.
The diff coverage is 97.5%.

@@             Coverage Diff              @@
##             master     #738      +/-   ##
============================================
+ Coverage     77.69%   83.09%   +5.39%     
- Complexity     1158     1261     +103     
============================================
  Files           222      225       +3     
  Lines          3349     3407      +58     
  Branches        191      195       +4     
============================================
+ Hits           2602     2831     +229     
+ Misses          700      529     -171     
  Partials         47       47

Impacted Files	Coverage Δ	Complexity Δ
...main/java/org/cactoos/scalar/InheritanceLevel.java	`100% <100%> (ø)`	`6 <6> (?)`
...rc/main/java/org/cactoos/scalar/CheckedScalar.java	`95.45% <95.45%> (ø)`	`6 <6> (?)`
src/main/java/org/cactoos/iterable/RangeOf.java	`81.81% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...n/java/org/cactoos/matchers/TeeInputHasResult.java	`100% <0%> (ø)`	`6% <0%> (?)`
src/main/java/org/cactoos/io/BytesOf.java	`94.2% <0%> (+2.89%)`	`29% <0%> (+1%)`	⬆️
src/main/java/org/cactoos/io/InputOf.java	`90% <0%> (+3.33%)`	`29% <0%> (+1%)`	⬆️
...main/java/org/cactoos/io/WriterAsOutputStream.java	`84% <0%> (+4%)`	`11% <0%> (+1%)`	⬆️
src/main/java/org/cactoos/io/ReaderAsBytes.java	`100% <0%> (+8.33%)`	`8% <0%> (+1%)`	⬆️
... and 5 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 2c5554b...258af34. Read the comment docs.

0crat · 2018-03-14T16:10:03Z

Job #738 is now in scope, role is REV

0crat · 2018-03-14T16:15:24Z

This pull request #738 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 @yegor256 /z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

Vatavuk · 2018-03-18T13:13:53Z

@fabriciofx ping

0crat · 2018-03-19T16:15:02Z

@fabriciofx /z this job was assigned to you 5 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

proshin-roman

Just a comment about static method

proshin-roman · 2018-03-22T13:02:58Z

src/main/java/org/cactoos/scalar/InheritanceLevel.java

+     * @param cderived Derived class
+     * @return Integer Level
+     */
+    private static int calculateLevel(final Class<?> cderived,


@Vatavuk this static method can be (and should be) refactored to non-static and then both input parameters will be available via this modifier.

@proshin-roman Thx for the comment, fixed.

0crat · 2018-03-22T16:15:42Z

@fabriciofx /z this job was assigned to you 8 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

0crat · 2018-03-24T16:16:12Z

The user @fabriciofx /z resigned from #738, please stop working. Reason for job resignation: It is older than 10 days, see §8

0crat · 2018-03-24T16:16:17Z

Resigned on delay, see §8: -15 points just awarded to @fabriciofx /z

0crat · 2018-03-24T16:19:50Z

This pull request #738 is assigned to @neonailol /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @yegor256 /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

proshin-roman · 2018-03-25T08:46:49Z

src/main/java/org/cactoos/scalar/CheckedScalar.java

+    /**
+     * Function that wraps exception.
+     */
+    private final Func<Exception, E> func;


@Vatavuk I believe wrapper would be the name exactly explains the purpose of this field. But I know about "no -er" rule... Damn :) Still func is not the best name. Could you try to find any better word?

@proshin-roman I changed the name to wrapped although there are many classes that use func as a field variable name. For example: SolidScalar, StickyScalar, RetryScalar...

proshin-roman · 2018-03-25T09:06:40Z

src/main/java/org/cactoos/scalar/CheckedScalar.java

+     * @return E Wrapped exception
+     */
+    @SuppressWarnings("unchecked")
+    private E wrappedException(final Exception exp) {


@Vatavuk I think the whole this logic about comparing exceptions is wrong and might lead to a problem: let's say I have this code

new CheckedScalar<Integer, IOException>( new ScalarOf(1), (ex) ->new IOException("New custom message", ex) );

In case of IOException thrown in scalar, your logic will erase my error message, but I wouldn't expect this. So, I would suggest to not add this extra-logic and just throw what the function returns.

@proshin-roman You are right. I introduced that logic because I was concerned about unnecessary boxing of exceptions but above example clearly reveals a side effect. I removed it.

Vatavuk · 2018-03-25T20:16:02Z

@neonailol What do you think about the above comments/changes?

proshin-roman · 2018-03-26T07:24:25Z

src/main/java/org/cactoos/scalar/CheckedScalar.java

+    /**
+     * Function that wraps exception.
+     */
+    private final Func<Exception, E> wrapped;


@Vatavuk wrapped is an object, that was wrapped, right? But we have an object that does this wrapping - wrapper :) I'm 100% sure it should be wrapper. Do you think we can use wrapper name?

@proshin-roman I don't agree, I want to name my objects/variables by what they are not by what they do. This variable represents wrapped exception.

Vatavuk · 2018-03-27T19:06:25Z

@neonailol Is this PR good to merge?

neonailol · 2018-03-27T21:45:49Z

@Vatavuk yes, all seems to be good

neonailol · 2018-03-27T21:45:56Z

@rultor good to merge

rultor · 2018-03-27T21:46:40Z

@rultor good to merge

@neonailol Thanks for your request. @yegor256 Please confirm this.

Vatavuk · 2018-03-27T23:08:21Z

@yegor256 @neonailol Maybe I've rushed this one a little. I am still concerned about unnecessary wrapping of identical type exceptions. I have an idea how to avoid this and still preserve error messages in the example above. Can I push a new commit?

Vatavuk · 2018-03-28T20:19:05Z

@neonailol Can you review the last commit. I apologize for the inconvenience but the solution occurred to me almost the same time you approved the changes.

yegor256 · 2018-03-30T05:06:37Z

src/main/java/org/cactoos/scalar/InheritanceLevel.java

+    /**
+     * Classes are not related.
+     */
+    private static final int NOT_RELATED = 999;


@Vatavuk why not Integer.MAX_VALUE? Would be more logical.

yegor256 · 2018-03-30T05:07:07Z

src/main/java/org/cactoos/scalar/InheritanceLevel.java

+    /**
+     * Classes are identical.
+     */
+    private static final int IDENTICAL = 0;


@Vatavuk I would remove this constant, it's not really helpful. Just use zero and MAX_VALUE.

yegor256 · 2018-03-30T05:07:53Z

@Vatavuk see above. In general, try to stay away from constants, they are indicators of wrong design. http://www.yegor256.com/2015/07/06/public-static-literals.html

Vatavuk · 2018-03-30T05:47:27Z

@yegor256 fixed. Thx for the link.

yegor256 · 2018-03-30T14:05:49Z

src/main/java/org/cactoos/scalar/CheckedScalar.java

+        final int level = new InheritanceLevel(
+            exp.getClass(), wrapped.getClass()
+        ).value();
+        final int unrelated = 999;


@Vatavuk this should be replaced with MAX_VALUE too, I believe

yegor256 · 2018-03-30T14:06:01Z

@Vatavuk see above

Vatavuk · 2018-03-30T14:50:25Z

@yegor256 fixed

0crat · 2018-03-30T15:14:29Z

@neonailol /z this job was assigned to you 6days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

0crat · 2018-03-30T18:35:05Z

@neonailol /z this job was assigned to you 6days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

neonailol

@rultor good to merge

Vatavuk · 2018-03-31T06:19:16Z

@yegor256 ping

yegor256 · 2018-04-02T18:35:54Z

@rultor merge

rultor · 2018-04-02T18:36:32Z

@rultor merge

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

rultor · 2018-04-02T18:45:54Z

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min)

0crat · 2018-04-02T19:00:24Z

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

0crat · 2018-04-02T19:00:31Z

The job #738 is now out of scope

0crat · 2018-04-02T19:00:34Z

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

elenavolokhova · 2018-04-02T20:15:46Z

@neonailol According to our Policy:

The code reviewer found at least three problems in the code

No issues were found by you in this case.
Please, confirm that you will try to find at least three problems next time.

neonailol · 2018-04-03T06:01:41Z

@elenavolokhova i confirm

elenavolokhova · 2018-04-03T08:46:40Z

@0crat quality bad

0crat · 2018-04-03T08:47:13Z

Quality is low, no payment, see §31

0crat · 2018-04-03T08:47:21Z

Quality review completed: +8 point(s) just awarded to @elenavolokhova /z

yegor256#735 CheckedScalar

946fe34

InheritanceLevel ctor fixed

f26ff2b

0crat added the scope label Mar 14, 2018

Vatavuk mentioned this pull request Mar 20, 2018

CheckedScalar #735

Closed

proshin-roman reviewed Mar 22, 2018

View reviewed changes

calculateLevel method refactored

a84cd14

proshin-roman reviewed Mar 25, 2018

View reviewed changes

InheritanceLevel removed

67ef496

proshin-roman reviewed Mar 26, 2018

View reviewed changes

neonailol approved these changes Mar 27, 2018

View reviewed changes

wrapping of identical exception types fixed

80346f9

yegor256 reviewed Mar 30, 2018

View reviewed changes

removed constants

c1ccfb4

yegor256 reviewed Mar 30, 2018

View reviewed changes

magic number replaced with Integer.MAX_VALUE

258af34

neonailol approved these changes Mar 30, 2018

View reviewed changes

rultor merged commit 258af34 into yegor256:master Apr 2, 2018

0crat removed the scope label Apr 2, 2018

Vatavuk deleted the 735 branch April 2, 2018 19:10

#735 CheckedScalar #738

#735 CheckedScalar #738

Conversation

Vatavuk commented Mar 13, 2018 • edited Loading

codecov-io commented Mar 13, 2018 • edited Loading

Codecov Report

0crat commented Mar 14, 2018

0crat commented Mar 14, 2018

Vatavuk commented Mar 18, 2018

0crat commented Mar 19, 2018

proshin-roman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0crat commented Mar 22, 2018

0crat commented Mar 24, 2018

0crat commented Mar 24, 2018

0crat commented Mar 24, 2018

proshin-roman Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vatavuk Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Vatavuk commented Mar 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vatavuk commented Mar 27, 2018

neonailol commented Mar 27, 2018

neonailol commented Mar 27, 2018

rultor commented Mar 27, 2018

Vatavuk commented Mar 27, 2018 • edited Loading

Vatavuk commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yegor256 commented Mar 30, 2018

Vatavuk commented Mar 30, 2018

yegor256 Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

yegor256 commented Mar 30, 2018

Vatavuk commented Mar 30, 2018

0crat commented Mar 30, 2018

0crat commented Mar 30, 2018

neonailol left a comment

Choose a reason for hiding this comment

Vatavuk commented Mar 31, 2018

yegor256 commented Apr 2, 2018

rultor commented Apr 2, 2018

rultor commented Apr 2, 2018

0crat commented Apr 2, 2018

0crat commented Apr 2, 2018

0crat commented Apr 2, 2018

elenavolokhova commented Apr 2, 2018

neonailol commented Apr 3, 2018

elenavolokhova commented Apr 3, 2018

0crat commented Apr 3, 2018

0crat commented Apr 3, 2018

Vatavuk commented Mar 13, 2018 •

edited

Loading

codecov-io commented Mar 13, 2018 •

edited

Loading

proshin-roman Mar 25, 2018 •

edited

Loading

Vatavuk Mar 25, 2018 •

edited

Loading

Vatavuk commented Mar 27, 2018 •

edited

Loading

yegor256 Mar 30, 2018 •

edited

Loading