Code review comment for lp:~matsubara/launchpad/update-ec2-merge-workflow

(15:27:58) adeuring: matsubara: in lines 48..55 of the diff: if both (--no-qa or --incr) and --rollback=... are defined, the former options are ignored. Should we issue are warning in this case?
(15:29:37) salgado-brb heißt jetzt salgado
(15:30:57) matsubara: adeuring, you mean like raising an exception if --no-qa=True and -- incr=True and --rollback=123?
(15:31:15) matsubara: adeuring, sounds like a good idea, yes
(15:31:47) adeuring: matsubara: I don't know if this is worth an exception, just printing a user warning might be enough. But I leave decision to you ;)
(15:32:51) matsubara: adeuring, ok, I'll go with the exception as this is the same pattern used by the other options.
(15:32:59) adeuring: matsubara: OK
(15:41:24) adeuring: matsubara: in line 109, you catch a TypeError, probably raised in line 55, or somewhere in the option parser. But TypeErrors are quite generic -- could you move the "try: ... except TypeError:" directly to the place(s) where it occurs and raise the BzrComandError there? TypeError are so generic, and catching them not very close to the code where they are expected can hide completely unrelated errors.
(15:46:24) matsubara: adeuring, just a sec, I'm on the stand up call
(15:51:48) matsubara: adeuring, moving BzrCommandError to autoland.py wouldn't make much sense. I just tried something else instead and I think I can remove the exception handling
(15:52:19) adeuring: matsubara: ok
(15:52:48) matsubara: adeuring, since 67..71 I'm defining the --rollback option as int, if I pass something else like --rollback=foo, the option parser will return me a nice error message
(15:53:19) matsubara: so, I'll remove the exception handling from there as it's unecessary in that case.
(15:54:22) adeuring: matsubara: right, I wandered too if it was even necessary. (BTW, What happens for a missing parameter value)
(15:55:37) matsubara: adeuring, this https://pastebin.canonical.com/36722/
(15:57:46) adeuring: matsubara: so, that's an optparse issue -- let's not worry about it. I was just curious if this might cause the type error in 'rolback=%d'' % rollbak
(15:58:51) matsubara: adeuring, yep, I was being extra careful catching that TypeError as I thought 'rollback=%d'' % rollback' would raise that, but then optparse will take care of that for us.
(15:59:07) adeuring: exaxtly
(16:02:01) adeuring: matsubara: so, with "except TypeError:" removed and the other change about the options --no-qa and --rollback, r=me