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.
|
| @@ -27,10 +27,34 @@ def run(args = []) | |||
| mine = Args.new(opts, @log).take || return | |||
| 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) |
There was a problem hiding this comment.
@botanicus do tests exist for these?
|
@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
#323
Note: #322 should be merged in first.