#61: Implement payment taxes to remote nodes. #72

paulodamaso · 2018-08-09T22:00:02Z

For #61: Implement payment taxes to remote nodes:

Added FkTransaction class to fake transaction in tests;
Updated puzzle in Taxes
Added test basic implementation for Taxes.exec()
Added puzzle for Transaction matcher implementation
Added puzzle for payToRightNodes implementation

codecov-io · 2018-08-09T22:03:51Z

Codecov Report

Merging #72 into master will decrease coverage by 4.55%.
The diff coverage is 56.25%.

@@             Coverage Diff              @@
##             master      #72      +/-   ##
============================================
- Coverage     93.71%   89.16%   -4.56%     
- Complexity       52       58       +6     
============================================
  Files            11       12       +1     
  Lines           191      240      +49     
  Branches         16       23       +7     
============================================
+ Hits            179      214      +35     
- Misses           11       22      +11     
- Partials          1        4       +3

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/io/zold/api/Taxes.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
src/main/java/io/zold/api/Transaction.java	`56.25% <56.25%> (ø)`	`0 <0> (?)`
src/main/java/io/zold/api/CpTransaction.java	`0% <0%> (-100%)`	`0% <0%> (-1%)`
src/main/java/io/zold/api/Wallet.java	`80.85% <0%> (-8.52%)`	`0% <0%> (ø)`
src/main/java/io/zold/api/RtTransaction.java	`98.76% <0%> (-1.24%)`	`26% <0%> (+1%)`
src/main/java/io/zold/api/Copies.java	`85.71% <0%> (+7.93%)`	`6% <0%> (+5%)`	⬆️
src/main/java/io/zold/api/RtNetwork.java	`100% <0%> (+10%)`	`6% <0%> (+1%)`	⬆️
src/main/java/io/zold/api/Remote.java	`87.5% <0%> (+12.5%)`	`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 6c86827...3aca978. Read the comment docs.

0crat · 2018-08-09T22:05:15Z

Job #72 is now in scope, role is REV

paulodamaso · 2018-08-14T17:15:10Z

@llorllale Is 0crat having problems in assigning REV tasks? This task is five days old, it's rotting and 0crat does not assign it to anyone.

llorllale · 2018-08-16T13:45:38Z

@paulodamaso funding issue - let me try to fix

paulodamaso · 2018-11-28T11:15:03Z

@llorllale Would you please review and merge this one? This task is preventing me from receiving new tasks, and I've made this PR more than two months ago.

llorllale · 2018-12-02T15:41:09Z

pom.xml

@@ -113,7 +113,7 @@ SOFTWARE.
          <plugin>
            <groupId>org.jacoco</groupId>
            <artifactId>jacoco-maven-plugin</artifactId>
-            <version>0.8.0</version>
+            <version>0.8.1</version>


@paulodamaso why are we bumping this version?

llorllale · 2018-12-02T15:42:51Z

src/main/java/io/zold/api/FkTransaction.java

+ * @checkstyle ParameterNumberCheck (500 lines)
+ */
+@SuppressWarnings("PMD.AvoidFieldNameMatchingMethodName")
+public final class FkTransaction implements Transaction {


@paulodamaso let's define this one inside Transaction and just call it Fake

llorllale · 2018-12-02T15:44:11Z

src/main/java/io/zold/api/FkTransaction.java

+ * Fake {@link Transaction} for usasge in tests.
+ *
+ * @since 1.0
+ * @checkstyle JavadocMethodCheck (500 lines)


@paulodamaso let's enable this and add javadocs to methods

llorllale · 2018-12-02T15:45:55Z

src/main/java/io/zold/api/FkTransaction.java

+ *
+ * @since 1.0
+ * @checkstyle JavadocMethodCheck (500 lines)
+ * @checkstyle ParameterNumberCheck (500 lines)


@paulodamaso let's enable this check and figure out how to fix that ctor with all those parameters. There's just too many of them for a single ctor.

@llorllale Considering that it is just a fake implementation of Transaction meant for using in tests I don't see this issue as being so grave. Besides, I've already spent too much on this ticket previously so I'll left a todo for this fix.

@paulodamaso please leave a puzzle

llorllale · 2018-12-02T15:49:44Z

src/test/java/io/zold/api/TaxesTest.java

+            ).value();
+        final List<Transaction> ledger = new ArrayList<>(5);
+        // @checkstyle AvoidInstantiatingObjectsInLoops (26 lines)
+        for (int index = 0; index < 5; index = index + 1) {


@paulodamaso you can just index++ on the post-iteration operation.

@llorllale Static checks fail if we use ++: Checkstyle: C:\git\zold-java-api\src\test\java\io\zold\api\TaxesTest.java[62]: Using '++' is not allowed. (IllegalTokenCheck)

@paulodamaso huh???!? wow

@paulodamaso pre-increment should work

llorllale · 2018-12-02T15:52:24Z

src/test/java/io/zold/api/TaxesTest.java

+                "2017-07-19T21:24:51Z",
+                DateTimeFormatter.ISO_OFFSET_DATE_TIME
+            ).value();
+        final List<Transaction> ledger = new ArrayList<>(5);


@paulodamaso why are we specifying capacity on this list?

@llorllale see comment below

llorllale · 2018-12-02T15:52:59Z

src/test/java/io/zold/api/TaxesTest.java

+                )
+            );
+        }
+        final List<Remote> remotes = new ArrayList<>(5);


@paulodamaso why are we specifying capacity on this list?

@llorllale Checkstyle fails if we do not so: Checkstyle: C:\git\zold-java-api\src\test\java\io\zold\api\TaxesTest.java[83]: ArrayList should be initialized with a size parameter (ConditionalRegexpMultilineCheck)

@paulodamaso Christ.... wow. Thanks.

@paulodamaso yegor256/qulice#942

llorllale · 2018-12-02T15:53:19Z

src/test/java/io/zold/api/TaxesTest.java

+            );
+        }
+        final List<Remote> remotes = new ArrayList<>(5);
+        for (int counter = 0; counter < 5; counter = counter + 1) {


@paulodamaso just use counter++ to increment counter.

Also, I think the loop's exit condition should be rewritten as counter < ledger.size()

@llorllale Please see above about ++ operator.

paulodamaso

@llorllale Thanks for the review, made some corrections and comments, please take a look

paulodamaso · 2018-12-02T16:52:54Z

src/test/java/io/zold/api/TaxesTest.java

+            ).value();
+        final List<Transaction> ledger = new ArrayList<>(5);
+        // @checkstyle AvoidInstantiatingObjectsInLoops (26 lines)
+        for (int index = 0; index < 5; index = index + 1) {


@llorllale Static checks fail if we use ++: Checkstyle: C:\git\zold-java-api\src\test\java\io\zold\api\TaxesTest.java[62]: Using '++' is not allowed. (IllegalTokenCheck)

paulodamaso · 2018-12-02T16:54:16Z

src/test/java/io/zold/api/TaxesTest.java

+            );
+        }
+        final List<Remote> remotes = new ArrayList<>(5);
+        for (int counter = 0; counter < 5; counter = counter + 1) {


@llorllale Please see above about ++ operator.

paulodamaso · 2018-12-02T17:02:33Z

src/test/java/io/zold/api/TaxesTest.java

+                )
+            );
+        }
+        final List<Remote> remotes = new ArrayList<>(5);


@llorllale Checkstyle fails if we do not so: Checkstyle: C:\git\zold-java-api\src\test\java\io\zold\api\TaxesTest.java[83]: ArrayList should be initialized with a size parameter (ConditionalRegexpMultilineCheck)

paulodamaso · 2018-12-02T17:59:32Z

src/test/java/io/zold/api/TaxesTest.java

+                "2017-07-19T21:24:51Z",
+                DateTimeFormatter.ISO_OFFSET_DATE_TIME
+            ).value();
+        final List<Transaction> ledger = new ArrayList<>(5);


@llorllale see comment below

paulodamaso · 2018-12-02T18:01:55Z

src/main/java/io/zold/api/FkTransaction.java

+ *
+ * @since 1.0
+ * @checkstyle JavadocMethodCheck (500 lines)
+ * @checkstyle ParameterNumberCheck (500 lines)


@llorllale Considering that it is just a fake implementation of Transaction meant for using in tests I don't see this issue as being so grave. Besides, I've already spent too much on this ticket previously so I'll left a todo for this fix.

llorllale · 2018-12-02T21:50:31Z

src/test/java/io/zold/api/TaxesTest.java

+        MatcherAssert.assertThat(
+            "Didn't paid anything",
+            wallet.ledger(),
+            new IsCollectionContaining<Transaction>(


@paulodamaso is explicit generic type parameter necessary?

paulodamaso · 2018-12-03T00:12:35Z

@llorllale Done take a look now

llorllale · 2018-12-04T00:43:25Z

@rultor merge

rultor · 2018-12-04T00:44:34Z

@rultor merge

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

rultor · 2018-12-04T00:55:24Z

@rultor merge

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

0crat · 2018-12-04T01:19:21Z

Job gh:zold-io/java-api#72 is not assigned, can't get performer

0crat · 2018-12-04T01:22:13Z

The job #72 is now out of scope

For zold-io#61: Implement payment taxes to remote nodes.

27d1651

0crat added the scope label Aug 9, 2018

For zold-io#61: Resolving conflicts.

695cbbb

paulodamaso mentioned this pull request Aug 13, 2018

Taxes.java:34-40: Implement tax payment to remote nodes.... #61

Closed

llorllale reviewed Dec 2, 2018

View reviewed changes

For zold-io#61: review fixes.

0638a80

paulodamaso commented Dec 2, 2018

View reviewed changes

For zold-io#61: review fixes.

dc016b7

llorllale reviewed Dec 2, 2018

View reviewed changes

For zold-io#61: review fixes.

3aca978

rultor merged commit 3aca978 into zold-io:master Dec 4, 2018

0crat removed the scope label Dec 4, 2018

#61: Implement payment taxes to remote nodes. #72

#61: Implement payment taxes to remote nodes. #72

Conversation

paulodamaso commented Aug 9, 2018

codecov-io commented Aug 9, 2018 • edited

Codecov Report

0crat commented Aug 9, 2018

paulodamaso commented Aug 14, 2018

llorllale commented Aug 16, 2018

paulodamaso commented Nov 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Dec 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Dec 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulodamaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulodamaso commented Dec 3, 2018

llorllale commented Dec 4, 2018

rultor commented Dec 4, 2018

rultor commented Dec 4, 2018

0crat commented Dec 4, 2018

0crat commented Dec 4, 2018

codecov-io commented Aug 9, 2018 •

edited

llorllale Dec 2, 2018 •

edited

llorllale Dec 2, 2018 •

edited