Improved user-facing messages

Improved user-facing messages

Updated some (pylint) comments/docstrings.

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=244775

[pgervais, 2013-12-28 01:01:12]

The sensitive modification is in PendingManager, where I replaced an assert by a
if+raise to use the standard python exception instead of a generic assertion
error.

[ghost stip (do not use), 2014-01-02 20:56:33]

https://chromiumcodereview.appspot.com/102703004/diff/1/tests/pending_manager...
File tests/pending_manager_test.py (right):

https://chromiumcodereview.appspot.com/102703004/diff/1/tests/pending_manager...
tests/pending_manager_test.py:169: except ValueError:
I'm not sure I understand what's going on here. Can you walk me through it?

[pgervais, 2014-01-02 22:16:47]

https://chromiumcodereview.appspot.com/102703004/diff/1/tests/pending_manager...
File tests/pending_manager_test.py (right):

https://chromiumcodereview.appspot.com/102703004/diff/1/tests/pending_manager...
tests/pending_manager_test.py:169: except ValueError:
On 2014/01/02 20:56:33, stip wrote:
> I'm not sure I understand what's going on here. Can you walk me through it?

_get_pc instantiate a PendingManager using no verifiers (thus the empty list.)
This must trigger an exception. I did two things:
- move the code that displays an error message in a "else" section, because
every exception that self.fail raises have no reason to be excepted.
- replaced in PendingManager an invocation of assert by a proper test and a
raise ValueError, because it's more appropriate than assert.

Changing the exception raised has possibly some consequence on existing code,
but since catching AssertionError is hardly done, that shouldn't be a problem.

[ghost stip (do not use), 2014-01-03 22:36:19]

lgtm

[pgervais, 2014-01-03 22:37:50]

Following discussion on CL #117513003, I removed every pylint comments I had
added.

+pawel

[Paweł Hajdan Jr., 2014-01-06 11:23:25]

LGTM with comments, I don't want to block you - just make sure to apply them and
get another round of review on any non-trivial changes or if you'd rather not
apply some of them.

https://codereview.chromium.org/102703004/diff/70001/async_push.py
File async_push.py (right):

https://codereview.chromium.org/102703004/diff/70001/async_push.py#newcode114
async_push.py:114: url = self.url + '/receiver'   # application-specific
nit: It's not obvious what this comment adds.

I'd rather make this /receiver path part also part of the ctor, so that the
class itself is not application specific.

https://codereview.chromium.org/102703004/diff/70001/pending_manager.py
File pending_manager.py (right):

https://codereview.chromium.org/102703004/diff/70001/pending_manager.py#newco...
pending_manager.py:161: % project_name)
You may also want to throw an exception if project_name is empty.

project_name has a default value, and people will skip things that are not
strictly required to make something work.

https://codereview.chromium.org/102703004/diff/70001/pending_manager.py#newco...
pending_manager.py:436: logging.error(str(e))
Could you explain more why? Please note logging.error call above.

https://codereview.chromium.org/102703004/diff/70001/tests/pending_manager_te...
File tests/pending_manager_test.py (right):

https://codereview.chromium.org/102703004/diff/70001/tests/pending_manager_te...
tests/pending_manager_test.py:182: self.fail(msg="A PendingManager should not
accept the same verifier"
nit: Add a space between "verifier" and "two".

By the way, do you need to say msg= explicitly when calling self.fail?

[pgervais, 2014-01-06 22:22:58]

I answered to some of you comments, and will upload a new CL soon.

https://codereview.chromium.org/102703004/diff/70001/pending_manager.py
File pending_manager.py (right):

https://codereview.chromium.org/102703004/diff/70001/pending_manager.py#newco...
pending_manager.py:161: % project_name)
On 2014/01/06 11:23:26, Paweł Hajdan Jr. wrote:
> You may also want to throw an exception if project_name is empty.
> 
> project_name has a default value, and people will skip things that are not
> strictly required to make something work.

I added 'project_name' as a convenience, and used it only in one place (the
future bling CQ). It's optional just to be backward-compatible, I didn't want to
risk breaking every commit queue by modifying all of them at once. However, if
you feel that it's worth it, I can do it.

https://codereview.chromium.org/102703004/diff/70001/pending_manager.py#newco...
pending_manager.py:436: logging.error(str(e))
On 2014/01/06 11:23:26, Paweł Hajdan Jr. wrote:
> Could you explain more why? Please note logging.error call above.

str(e) gives the full HTTP status, which is not visible in the stack trace.

https://codereview.chromium.org/102703004/diff/70001/tests/pending_manager_te...
File tests/pending_manager_test.py (right):

https://codereview.chromium.org/102703004/diff/70001/tests/pending_manager_te...
tests/pending_manager_test.py:182: self.fail(msg="A PendingManager should not
accept the same verifier"
On 2014/01/06 11:23:26, Paweł Hajdan Jr. wrote:
> nit: Add a space between "verifier" and "two".
> 
> By the way, do you need to say msg= explicitly when calling self.fail?

No, it's optional. However, should the signature of self.fail change in the
future, it would be better to have used keyword argument names instead of
relying on their position.

I haven't found anything related in the Google python style guide.

[pgervais, 2014-01-08 22:56:15]

Uploaded a new CL, can I have a final opinion on this?

[ghost stip (do not use), 2014-01-09 00:46:28]

lgtm % nits

https://chromiumcodereview.appspot.com/102703004/diff/70001/pending_manager.py
File pending_manager.py (right):

https://chromiumcodereview.appspot.com/102703004/diff/70001/pending_manager.p...
pending_manager.py:161: % project_name)
On 2014/01/06 22:22:58, pgervais wrote:
> On 2014/01/06 11:23:26, Paweł Hajdan Jr. wrote:
> > You may also want to throw an exception if project_name is empty.
> > 
> > project_name has a default value, and people will skip things that are not
> > strictly required to make something work.
> 
> I added 'project_name' as a convenience, and used it only in one place (the
> future bling CQ). It's optional just to be backward-compatible, I didn't want
to
> risk breaking every commit queue by modifying all of them at once. However, if
> you feel that it's worth it, I can do it.

You have to specify the project when you start up commit-queue, so I'm not sure
what good having the project name in the error message is if it'll be blank most
of the time.

https://chromiumcodereview.appspot.com/102703004/diff/160001/tests/pending_ma...
File tests/pending_manager_test.py (right):

https://chromiumcodereview.appspot.com/102703004/diff/160001/tests/pending_ma...
tests/pending_manager_test.py:182: self.fail(msg="A PendingManager should not
accept the same verifier"
as paweł spotted, put a space between verifier and two

[pgervais, 2014-01-09 01:03:46]

On 2014/01/09 00:46:28, stip wrote:

> > I added 'project_name' as a convenience, and used it only in one place (the
> > future bling CQ). It's optional just to be backward-compatible, I didn't
want
> to
> > risk breaking every commit queue by modifying all of them at once. However,
if
> > you feel that it's worth it, I can do it.
> 
> You have to specify the project when you start up commit-queue, so I'm not
sure
> what good having the project name in the error message is if it'll be blank
most
> of the time.

My current experience with the bling infrastructure is the more info you have in
the log
files, the better. I'm adding this keyword right now, projects can use it later
on. I can
help with that, it's really simple.

[ghost stip (do not use), 2014-01-13 19:11:25]

ok, I'm fine with this patch as long as you add the space in

[ghost stip (do not use), 2014-01-13 22:43:18]

it's a race condition with rietveld. iannucci@ can tell you more, but it's
harmless (although annoying).

I found one more stupid nit, go ahead and fix and ship!

https://chromiumcodereview.appspot.com/102703004/diff/250001/tests/pending_ma...
File tests/pending_manager_test.py (right):

https://chromiumcodereview.appspot.com/102703004/diff/250001/tests/pending_ma...
tests/pending_manager_test.py:172: 
one last nit: ' instead of "

https://chromiumcodereview.appspot.com/102703004/diff/250001/tests/pending_ma...
tests/pending_manager_test.py:182: 
same here

[pgervais, 2014-01-14 01:08:10]

Final fix. I'll commit that tomorrow.

[commit-bot: I haz the power, 2014-01-14 18:37:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pgervais@chromium.org/102703004/250002

[commit-bot: I haz the power, 2014-01-14 18:38:07]

Change committed as 244775