Related to https://pagure.io/koji/issue/302
Why not just this?
https://github.com/mikem23/koji-playground/commits/issue302b
1 new commit added
I would say, that it is better to behave same in all similar cases. There is no strong argument for, just to be more intuitive.
Question is if sleep(60) should be same as options.poll_interval, but I would say so from the same reason.
hm... The default poll_interval is 6s I think. The wait_repo handler used 60 because repo regens aren't particularly fast. OTOH, sometimes they can be and that sounds more consistent.
I think my version does that. Why wrap the list-history handler in a try..except to catch KeyboardInterrupt, when we can just do that at the top level?
rebased
Makes sense - I've rewritten it.
unit test fails. watch_tasks also returns 1 if one of the tasks fails. We need to preserve that
Hmm, so we probably want to create special exit code for KeyboardInterrupt. I hasn't been able to find anything 'standard' enough. Closest is bash's 130 (http://www.tldp.org/LDP/abs/html/exitcodes.html) Any better idea?
KeyboardInterrupt
Do we need to return a special code? I'm not sure if anyone cares to distinguish between the two cases.
Note that python itself exits with code 1 if KeyboardInterrupt is not caught by the script.
Misunderstood the problem. I've returned that value and rebased to current master.
Some more comments?
So... this is very strange.
When I run make test on your branch, a bunch of unit tests are missed, dropping us down to 12% coverage. However, I cannot see any reason why this would be the case. The only changes on the branch are in cli/koji.
make test
cli/koji
I've rechecked this several times, even using a fresh clone.
Here's where I am:
aaf6fa8 (HEAD -> issue302, tkopecek/issue302) fix return value for watch_tasks aa80a63 unify KeyboardInterrupt behaviour for watch commands 882316c PR#336 use alabaster theme for docs
If I check out 882316c, the tests run as expected. At aa80a63, there is a loss of coverage and a failure. At aaf6fa8, the failure is gone, but the loss of coverage remains the same.
Less tests are being run.
-Ran 355 tests in 12.153s +Ran 116 tests in 1.321s
The loss of coverage is not in cli/koji, but spread out all over
AHHHH. Found it. There is a "Raise KeyboardInterrupt inner watch_tasks" test that now appears to terminate testing, presumably because the cli no longer catches it.
rebased here with unit test fixes: https://github.com/mikem23/koji-playground/commits/issue302
I have filed a bug against the test harness
https://github.com/nose-devs/nose/issues/1040
Confirming that tests are fixed by this.
Commit f499a448 fixes this pull-request
Pull-Request has been merged by mikem@redhat.com
Related to https://pagure.io/koji/issue/302