Implement partially WalletsIn.create #55
Conversation
|
Job #55 is now in scope, role is |
|
Job #55 is already in scope |
|
This pull request #55 is assigned to @carlosmiranda/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/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 a monetary reward for this job |
| } | ||
|
|
||
| @Test | ||
| public void createsWalletInWallets() throws IOException { |
There was a problem hiding this comment.
@victornoel let's name this returnsCreatedWallets.
| MatcherAssert.assertThat( | ||
| "Can't create wallet in wallets", | ||
| wallets, | ||
| new IsIterableWithSize<>(new IsEqual<>(1)) |
There was a problem hiding this comment.
@victornoel can we check the IDs of the returned wallets as well?
That said I know this might not be working yet, but let's add that to the puzzle to expand this test.
There was a problem hiding this comment.
@carlosmiranda that bit of code is not related to the PR, I just updated the style of the assertion to something more OO.
There was a problem hiding this comment.
@victornoel understood.
|
|
||
| /** | ||
| * Wallets in path. | ||
| * | ||
| * @since 0.1 | ||
| * @todo #12:30min Make uniform the way file extensions are handled |
There was a problem hiding this comment.
@victornoel not sure why we need a separate task here, should be fairly straightforward. Refactor the class by storing the extension as an instance variable, move the filter from the instance to a local variable to iterator(), and use the extension both in iterator and in create.
There was a problem hiding this comment.
@carlosmiranda do you mean I should have done that? I don't consider it's part of the scope of issue #12.
There was a problem hiding this comment.
@victornoel why not in the scope? It's at least somewhat related, it affects your implementation and the way wallets are fetched. Aside from that, it's an internal detail that's a) not defined in some architectural documentation (as far as I know), and b) easy to fix. An extra task seems superfluous to me.
There was a problem hiding this comment.
@carlosmiranda because I spent the allotted time on this, and the original issue wasn't about having a coherent naming of the wallet files, it was about creating a new wallet.
There was a problem hiding this comment.
@victornoel ok, as far as changes to this go, I'm all good. I'll approve it and let @llorllale decide what to do further.
There was a problem hiding this comment.
@victornoel I agree with @carlosmiranda on this: it was clear that the file extension for wallets is .z and it also looks like something easily handled internally as said here.
There was a problem hiding this comment.
@carlosmiranda @llorllale ok I will implement that
|
@carlosmiranda/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
victornoel
force-pushed
the
12-create-wallet
branch
from
33bcd47
to
0f6f44c
Compare
|
@carlosmiranda see answers to your comment. I updated the test name. |
Codecov Report
@@ Coverage Diff @@
## master #55 +/- ##
============================================
+ Coverage 92.81% 93.03% +0.22%
Complexity 49 49
============================================
Files 11 11
Lines 153 158 +5
Branches 10 10
============================================
+ Hits 142 147 +5
Misses 11 11
Continue to review full report at Codecov.
|
|
@victornoel thanks, please see my reply. |
|
@carlosmiranda likewise |
| */ | ||
| Wallet create(); | ||
| Wallet create(final long id) throws IOException; |
There was a problem hiding this comment.
@victornoel see my thoughts here. I think it's best for the id to be generated internally. I know that in zold-io/zold they have that separate Id class but I just don't see the need for that here at the moment.
There was a problem hiding this comment.
@llorllale good for me
| @@ -36,6 +41,11 @@ | |||
| * Test case for {@link WalletsIn}. | |||
| * | |||
| * @since 0.1 | |||
| * @todo #12:30min The name of the wallets in the test | |||
There was a problem hiding this comment.
@victornoel the naming of the test wallets are OK for now.
a test should be added to ensure WalletsIn.create does overwrite and existing wallet.
You mean does not overwrite an existing wallet?
There was a problem hiding this comment.
@llorllale yes, good catch :)
There was a problem hiding this comment.
@victornoel please remove the part about the test wallets naming. They're OK for testing purposes.
There was a problem hiding this comment.
@llorllale I don't understand, do you want me to remove the whole @todo or that I just leave a small one about testing not overwriting files? That's seem too small for a task and it will come naturally in the future.
There was a problem hiding this comment.
@victornoel well, you can remove the @todo and leave a stubbed test in WalletsInTest
There was a problem hiding this comment.
@llorllale what do you mean stubbed test? I can't test for overwriting since anyway ids are randomly generated. I will just remove the todo and then the problem will manifest itself naturally when it should I suppose.
There was a problem hiding this comment.
@victornoel I still think it's a valid test. All I think you need to do is add the skeleton of a test that would check this (make sure you leave a @todo for it).
There was a problem hiding this comment.
@llorllale alright
|
@victornoel see my comments above |
victornoel
force-pushed
the
12-create-wallet
branch
from
0f6f44c
to
89cfd44
Compare
|
@llorllale @carlosmiranda I've pushed corrections |
victornoel
force-pushed
the
12-create-wallet
branch
2 times, most recently
from
5623d34
to
f35a101
Compare
|
@llorllale I removed the |
|
@llorllale I added an ignored test for non-overwriting with some ideas about how to check it. |
victornoel
force-pushed
the
12-create-wallet
branch
from
f35a101
to
9a58549
Compare
|
@llorllale actually wait, I'm going to improve that test |
victornoel
force-pushed
the
12-create-wallet
branch
from
9a58549
to
145910a
Compare
|
@llorllale it is safer now :) |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale @victornoel Oops, I failed. You can see the full log here (spent 10min) |
victornoel
force-pushed
the
12-create-wallet
branch
from
145910a
to
cb44a08
Compare
|
@llorllale the |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale @victornoel Oops, I failed. You can see the full log here (spent 12min) |
|
@victornoel remove or change the string argument for the |
victornoel
force-pushed
the
12-create-wallet
branch
from
cb44a08
to
2c0e9a2
Compare
|
@llorllale my bad, thanks for the hint, I believe it should be ok now. |
|
@rultor merge |
@llorllale OK, I'll try to merge now. You can check the progress of the merge here |
@llorllale Done! FYI, the full log is here (took me 20min) |
|
Job |
|
@ypshenychka/z please review this job completed by @carlosmiranda/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #55 is now out of scope |
|
Payment to |
|
@carlosmiranda According to our QA Rules:
Only one issue was found during code review. |
|
@ypshenychka confirmed |
|
@carlosmiranda thank you |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @carlosmiranda/z |
|
Quality review completed: +8 point(s) just awarded to @ypshenychka/z |
This is for #12:
Walletcreation but not the content of the file@todo)@todos to continue the work