#1320 - ScalarOf class created #1339
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request #1339 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 |
victornoel
requested changes
Apr 10, 2020
Codecov Report
@@ Coverage Diff @@
## master #1339 +/- ##
============================================
+ Coverage 89.57% 89.58% +0.01%
- Complexity 1652 1654 +2
============================================
Files 273 274 +1
Lines 3933 3937 +4
Branches 211 211
============================================
+ Hits 3523 3527 +4
- Misses 376 377 +1
+ Partials 34 33 -1
Continue to review full report at Codecov.
|
|
@marceloamadeu can you go on with this? |
|
@marceloamadeu ping? |
|
@victornoel Please check the tests and todo. |
|
@marceloamadeu now you need to remove the todo you are solving (in the |
|
@victornoel Done... tks |
victornoel
approved these changes
Apr 18, 2020
There was a problem hiding this comment.
@marceloamadeu thank you
@paulodamaso it's good to merge :)
paulodamaso
suggested changes
Apr 19, 2020
| /** | ||
| * The scalar. | ||
| */ | ||
| private final Scalar<T> origin; |
There was a problem hiding this comment.
@marceloamadeu This class seems a bit strange to me; shouldn't we be wrapping the T value, instead another scalar? @victornoel WDYT?
|
@victornoel As reviewer, I want you opinon on this |
|
@paulodamaso the problem is that the initial todo was about introducing A What is more problematic is that the public constructor takes a An alternative to solve the initial todo being solved here would be to have a |
|
@victornoel I think that what we got here is enough. I'd like to see an implementation of |
|
@paulodamaso in that case it won't be usable as in the tests modified in this PR since they are wrapping lambda throwing exceptions. Or do you mean adding another constructor that takes T? In that case yes it will be useful for sure. But it will increase ambiguity because what is the desired outcome of
|
|
@victornoel I was thinking in adding another constructor, but I am concerned with this ambiguity problem. Lets keep it the way it is for now |
|
@rultor merge |
1 similar comment
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@marceloamadeu @paulodamaso Oops, I failed. You can see the full log here (spent 3min) |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@marceloamadeu @paulodamaso Oops, I failed. You can see the full log here (spent 3min) |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@paulodamaso Done! FYI, the full log is here (took me 8min) |
|
@paulodamaso could you please tell 0crat out? it missed the event I think |
|
@0crat status |
|
@victornoel This is what I know about this job in C63314D6Z, as in §32:
|
|
@0crat out |
|
@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 |
|
Code review was too long (15 days), architects (@paulodamaso) were penalized, see §55 |
|
@victornoel please make sure you start all your comments with the name of the user they are referred to, see |
|
@sereshqua ha yes! thanks for the reminder :) |
|
@0crat quality acceptable |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is for #1320