Code review comment for lp:~allenap/maas/dont-crash-when-selecting-fpi--bug-1293676--1.5

Revision history for this message
Gavin Panella (allenap) wrote :

> This looks like a straight backport.  Feel free to self-approve those,
> assuming there are no conflicts.

I was thinking that Julian ought to be gatekeeper on what we're going to
spend time trying to get into Trusty from here on, but a consensus is
good too.

>
> However I do have a few notes about the code changes that also apply to what's
> already in trunk.  It might be worth addressing them, and then backporting the
> updated version of the changes:
>
> .
>
> The big warning strings strike me as a bit confusing.  Updated?  In what way?
> By whom?  Why?  These situations often start with the passive voice.  Don't do
> it.  The imperative is fine when you're telling the user something they need
> to do in order to accomplish something — you don't even need to say “please.”

I can see that might be an improvement, but it also strikes me as a tiny
one. I don't know if that's because English is my first language, or
because I'm me. The passive voice here comes across as more polite; I'm
not telling you what to do, but I am telling you what must be done to
achieve your goal.

>
> .
>
> Both the ‘inhibit’ methods in node_action.py have this weird indentation:
>
>             return dedent("""\
>             The use-fastpath-installer tag is defined with an
>
> Did tabs sneak in?  I would expect the text inside an argument to the function
> call to be indented further than the line that starts the function call.
>
> If that makes the indentation too deep for comfortable text flow, why not
> extract these two identical-looking methods, or parts thereof, into a function
> outside the classes?  In fact, with these two near-identical explanations both
> repeated, why not extract them into global constants?

I just reflexively used python-mode's default indentation for this, and
I'm happy with that tbh. I'd be happy with it another 4 spaces further
in too. My shed's purple, yours is aubergine :-/

I think the pain of another branch and another back-port far outweighs
the gain of fixing either or both of these issues. Sorry.

Thanks for being so thorough though!

« Back to merge proposal