Command handling implemented #323 #331
Conversation
| Job #331 is now in scope, role is |
| This pull request #331 is assigned to @ledestin/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; there will be a monetary reward for this job |
| @botanicus again no tests? |
| @botanicus each pull request must have a combination of "new tests + new code." Without tests a PR won't be merged. We simply can't be sure that the code you contribute works. |
| @yegor256 that makes sense, however here we already split the work in puzzle 1 test and puzzle 2 this. Makes sense? If not, what’s your suggestion? |
| @botanicus that was a mistake. Let's not make it worse. Just follow the rule: every change starts with a test. Doesn't matter where. |
| @yegor256 let’s do that. In order for me to do some progress there though, please reply to my comments in #330 Additionally: is what’s in the PR satisfactory to meet the requirements for having tests in this issue? And if not now, will it meet them once reworked in the desired way, or is there a need to do something in this PR directly? |
| @botanicus the rule is simple: I have to be sure that the code you submit works. It there are no tests -- I don't trust your code. |
| @botanicus not at all. That ticket will be merged by itself. Then, we will get back to this ticket and the question will still be the same: what is broken now? Every time you submit a pull request you are fixing something that is broken. If it's broken, you should prove that it's broken -- with a test. If you can't prove it's broken -- what's the point of the pull request? You are not fixing anything, right? Thus, every change comes to the master branch with a test that proves that something was broken and now it's fixed. |
Codecov Report
@@ Coverage Diff @@ ## master #331 +/- ## ========================================= - Coverage 33.76% 33.7% -0.06% ========================================= Files 50 49 -1 Lines 2343 2332 -11 ========================================= - Hits 791 786 -5 + Misses 1552 1546 -6
Continue to review full report at Codecov.
|
| command = mine[0] | ||
| raise "A command is required, try 'zold alias --help'" unless command | ||
| case command | ||
| when 'set' then set(mine[1], mine[2]) |
There was a problem hiding this comment.
@botanicus please don't make us guess what mine[1] is, extract variables.
There was a problem hiding this comment.
@ledestin it's how it's been used in other commands as well, for instance https://github.com/zold-io/zold/blob/master/lib/zold/commands/remote.rb#L120
If you think this is wrong, then it should be changed everywhere, not just here, in which case I think bug report is where it belongs.
I personally don't think there's anything wrong with it since they are named in both method signature and in the help just above it.
I'm not sure what @yegor256 thinks, I've seen something from him about avoiding unnecessary variables, but I cannot tell if this is one of the cases or not.
Anyhow ... since it's not specific to this code, I'd suggest a bug report. Makes sense?
| # The set command expects 'wallet' which is the wallet id | ||
| # and 'alias' (here called 'name' as alias is a Ruby keyword). | ||
| # It writes this info into the alias file as described in #279. | ||
| def set(_wallet, _name) |
| @botanicus please see my comments |
| @ledestin please see. |
| @botanicus again no tests? We can't merge anything without tests first. |
| @botanicus let's wait for them to be merged and see what happens? At the moment we can't merge this. |
| @botanicus will you continue here? |
| case command | ||
| when 'set' then set(mine[1], mine[2]) | ||
| when 'remove' then remove(mine[1]) | ||
| when 'show' then show(mine[1]) |
There was a problem hiding this comment.
else raise “Unknown command: ‘#{command}’” end 6 participants
#323
Note: #322 should be merged in first.