> 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.
> 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.
> installer tag is defined with an
> .
>
> Both the ‘inhibit’ methods in node_action.py have this weird indentation:
>
> return dedent("""\
> The use-fastpath-
>
> 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!