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

#1335: NumerEnvelope wrap function restored #1488

Closed
wants to merge 5 commits into from

Conversation

catdog905
Copy link

@catdog905 catdog905 commented Oct 21, 2020

public final double doubleValue() {
return new Unchecked<>(this.dnum).value();
public float floatValue() {
return origin.floatValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

@ruca905 Please qualify field access with this.

@@ -65,49 +65,26 @@ public NumberOf(final String txt) {
this(new TextOf(txt));
}

public NumberOf(final Scalar<Long> lnm, final Scalar<Integer> inm,
Copy link
Contributor

Choose a reason for hiding this comment

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

@ruca905 Could you place each argument on separate line?

@@ -235,7 +235,7 @@ public MaxOf(final Float... src) {
}
}
return max;
});
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

@ruca905 It would be match better not to cluster parenthesis this way

@andreoss
Copy link
Contributor

andreoss commented Oct 21, 2020

@ruca905 The build is failing, I believe you have many qulice complains as well. Also the topic of PR is not very clear, and the body lacks a link to the issue

andreoss added a commit to andreoss/cactoos that referenced this pull request Nov 1, 2020
@andreoss
Copy link
Contributor

andreoss commented Nov 1, 2020

@ruca905 Any update on this?

@catdog905 catdog905 changed the title #1335: NumerEnvelope became envelope, NumberOf cat take scalar #1335: NumerEnvelope wrap function restored Nov 6, 2020
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.

@ruca905 here are some last changes to finish this PR. Ping me when it's done :)

}

@Override
public final int intValue() {
return new Unchecked<>(this.inum).value();
public Double value() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruca905 please remove the implementation of Scalar<Double>, it's out of scope of NumberEnvelope.

this.fnum = fnm;
this.dnum = dnm;
public NumberEnvelope(final Number number) {
this.origin = number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruca905 please also delegate byteValue, shortValue, toString, equals and hashCode

final Scalar<Integer> inm,
final Scalar<Float> fnm,
final Scalar<Double> dnm) {
this(dnm);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruca905 I think there was some kind of confusion about what NumberOf and Sealed were about… so, now that it's clearer to me, I think we should merge Sealed and NumberOf into NumberOf, for this:

  • we move the old code based on multiple scalars from NumberEnvelope to NumberOf
  • we move the constructors of Sealed to NumberOf
  • we keep the text/string based constructor here and delegate to the scalar-based ones
  • we delete Sealed

@@ -67,7 +63,7 @@ public void readFromGzipInput() throws Exception {
).affirm();
}

@Test(expected = EOFException.class)
@Test(expected = UncheckedIOException.class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruca905 please revert all changes from this file

@@ -86,7 +81,7 @@ public void writeToGzipOutput() throws Exception {
).affirm();
}

@Test(expected = IOException.class)
@Test(expected = UncheckedIOException.class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ruca905 please revert all changes from this file

@victornoel victornoel closed this Dec 27, 2020
@0crat 0crat added the qa label Dec 27, 2020
@0crat
Copy link
Collaborator

0crat commented Dec 27, 2020

@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

@0crat 0crat removed the 0crat/scope label Dec 27, 2020
@0crat
Copy link
Collaborator

0crat commented Dec 27, 2020

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

@0crat
Copy link
Collaborator

0crat commented Dec 27, 2020

Pull request #1488 was not merged, no payment for ARC, see §28

@sereshqua
Copy link

@ruca905 please make sure that all problems addressed by code reviewers would be fixed by you in next PRs, thanks

@sereshqua
Copy link

@0crat quality acceptable

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.

5 participants