Launchpad itself

Code review comment for lp:~lifeless/launchpad/librarian

librarian
Merge into devel

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2010-10-26:

Hi Robert,

Hopefully none of what I say here will matter much for this branch, since I already approved it, but for completeness' sake:

> mmm, OTOH they couldn't land [much] earlier as the code being improved
> is an older iteration of the overall theme; seeing it makes the code
> using the fixtures easier to understand I think - because its both
> new, and not in devel yet anyway.
>
> I really appreciate you finding my half done comments, I think I was
> getting fatigued with the branch.

This is not at all uncommon. When a branch gets this many meaningful lines, the engineer often starts leaning on the reviewer more. There's no single quantitative criterion that will capture the perfect moment to cut off a branch, but the 800-line limit has served us reasonably well as a signal that it's time to break things down into smaller branches.

Firing off a preparatory branch is usually a productive thing to do at that point. It's a hassle getting the extra review, but thanks to pipelines and requisite branches it is possible to get simultaneous reviews for multiple branches. The reviews also tend to be lighter, partly because a bit of isolated attention soon shows on the preparatory changes.

> > === modified file 'daemons/librarian.tac'
> > --- daemons/librarian.tac 2010-10-20 18:43:29 +0000
> > +++ daemons/librarian.tac 2010-10-21 04:51:25 +0000

> > + self.restricted_download_port = \
> > + config.librarian.restricted_download_port
> > + self.restricted_upload_port = \
> > + config.librarian.restricted_upload_port
> >
> > Our style guides wants us to break lines using parentheses rather than
> backslashes:
> >
> > + self.restricted_download_port = (
> > + config.librarian.restricted_download_port)
> >
> > For reference, search for "\" in https://dev.launchpad.net/PythonStyleGuide
>
> mmm, thats actually often less clear when its not a tuple. Its cute
> that (foo) == foo in python, but you have to think harder about it.

I suspect that's largely a matter of taste; my first decade of C/C++ programming was enough to cure me of any sympathy whatsoever for the backslash approach. On the other hand, I do see enough trailing whitespace creep into our code to make them a rational concern.

> > @@ -186,21 +192,26 @@
> >
> > @property
> > def pidfile(self):
> > - return os.path.join(self.root, 'librarian.pid')
> > + try:
> > + return os.path.join(self.root, 'librarian.pid')
> > + except AttributeError:
> > + # Not setup and using dynamic config: tachandler reads this
> early.
> > + return '/tmp/unused/'
> >
> >
> > I'm having trouble understanding the comment. Is the "setup" a verb? Is
> the "and" perhaps meant to be "when"?
>
> The and is deliberate... How about this?
> # An attempt to read the pidfile before this fixture was setUp, with
> dynamic configuration
> # the pidfile location is undefined at this point in time.

I still have trouble understanding this. Is the comma meant to be a full stop? Is there punctuation missing at the end of the first line?

> > + # And it exposes a config fragment
> > + # (Which is not activated by it.
> >
> > Looks like this comment was never completed.
>
> Just a missing ), fwiw.

On a sidenote: the passive voice is best avoided¹. "Which is not activated by it" is just a convoluted way of saying "Which it does not activate." And "But does not activate it" would be even easier to follow.

(¹) Smiley face.

> What would be really nice would be lazr config supporting a 'now give
> me a fragment' idiom. E.g. - magical sketching here:
> config.push()
> config.librarian.upload_port = 2134
> ...
> self.service_config = config.pop_changes()

That's actually not that far removed from what IStackableConfig.pop does, AFAICT.

> > How big is chunks? If it's not too big, it might be easier (both in coding
> and in debugging) to just assertIn that 'daemon ready' occurs in
> '\n'.join(chunks).
>
> In this case I want to be sure that that message isn't split between
> chunks, because other code depends on it - so this is testing the
> desired contract.

I understand that; hence the '\n'. That does not occur in the string you're looking for, and so will reveal any splitting that may happen inside the string. I think the error output from a failure would be very helpful, since it would give the entire relevant text (with the chunk boundaries clearly visible).

> > Also, there's a small problem with indentation in these situations: the
> continuation of the "heading" of the code block (in this case the nested()) is
> indented at the same level as the body of the code block, with nothing
> inbetween.
> >
> > The review team didn't like that, so we came up with two alternative
> suggestions for getting around it:
> >
> > 1. Keep whatever you need for the "heading" in variables so it fits on one
> line.
> >
> > 2. Separate the "heading" and the block's body with a comment.
>
> Whats the motivation here? Its easily readable, visually distinctive
> (the ): line is a clear marker).

We aim for absolutely minimal overhead to reading and understanding the code. Think Kernighan's Law and barrier to entry, but also simply "drag." From that perspective, it was considered helpful for the reader to see the structure of the code clearly from a quick scan along the left-hand margin without (or before) mentally parsing the code.

(One of the main functions of our reviews is to ingrain and maintain consistent habits that serve these goals. It's actually this exact quality in our programming that keeps me happy to work on this codebase.)

> Ohm and the
> ("""\
> stuff
>
> is just how dedent is used, because it strips leading match spaces putting stuff on the first line is fail.

Ah yes, that is a bit of real nastiness in dedent. Personally I fail to see why it's not a bug.

I hate to say it, but I think it's still better to solve it with lstrip('\n'), in order to eliminate any shadow of a doubt for a future reader that there may be invisible-but-significant whitespace in the source code. You may be careful enough not to insert any, but the reader won't know who last edited the line.

Jeroen

Hi Robert,

Hopefully none of what I say here will matter much for this branch, since I already approved it, but for completeness' sake:

> mmm, OTOH they couldn't land [much] earlier as the code being improved
> is an older iteration of the overall theme; seeing it makes the code
> using the fixtures easier to understand I think - because its both
> new, and not in devel yet anyway.
> 
> I really appreciate you finding my half done comments, I think I was
> getting fatigued with the branch.

This is not at all uncommon.  When a branch gets this many meaningful lines, the engineer often starts leaning on the reviewer more.  There's no single quantitative criterion that will capture the perfect moment to cut off a branch, but the 800-line limit has served us reasonably well as a signal that it's time to break things down into smaller branches.

Firing off a preparatory branch is usually a productive thing to do at that point.  It's a hassle getting the extra review, but thanks to pipelines and requisite branches it is possible to get simultaneous reviews for multiple branches.  The reviews also tend to be lighter, partly because a bit of isolated attention soon shows on the preparatory changes.

> > === modified file 'daemons/librarian.tac'
> > --- daemons/librarian.tac       2010-10-20 18:43:29 +0000
> > +++ daemons/librarian.tac       2010-10-21 04:51:25 +0000

> > +            self.restricted_download_port = \
> > +                config.librarian.restricted_download_port
> > +            self.restricted_upload_port = \
> > +                config.librarian.restricted_upload_port
> >
> > Our style guides wants us to break lines using parentheses rather than
> backslashes:
> >
> > +            self.restricted_download_port = (
> > +                config.librarian.restricted_download_port)
> >
> > For reference, search for "\" in https://dev.launchpad.net/PythonStyleGuide
> 
> mmm, thats actually often less clear when its not a tuple. Its cute
> that (foo) == foo in python, but you have to think harder about it.

I suspect that's largely a matter of taste; my first decade of C/C++ programming was enough to cure me of any sympathy whatsoever for the backslash approach.  On the other hand, I do see enough trailing whitespace creep into our code to make them a rational concern.

> > @@ -186,21 +192,26 @@
> >
> >     @property
> >     def pidfile(self):
> > -        return os.path.join(self.root, 'librarian.pid')
> > +        try:
> > +            return os.path.join(self.root, 'librarian.pid')
> > +        except AttributeError:
> > +            # Not setup and using dynamic config: tachandler reads this
> early.
> > +            return '/tmp/unused/'
> >
> >
> > I'm having trouble understanding the comment.  Is the "setup" a verb?  Is
> the "and" perhaps meant to be "when"?
> 
> The and is deliberate... How about this?
> # An attempt to read the pidfile before this fixture was setUp, with
> dynamic configuration
> # the pidfile location is undefined at this point in time.

I still have trouble understanding this.  Is the comma meant to be a full stop?  Is there punctuation missing at the end of the first line?

> > +                # And it exposes a config fragment
> > +                # (Which is not activated by it.
> >
> > Looks like this comment was never completed.
> 
> Just a missing ), fwiw.

On a sidenote: the passive voice is best avoided¹.  "Which is not activated by it" is just a convoluted way of saying "Which it does not activate."  And "But does not activate it" would be even easier to follow.

(¹) Smiley face.

That's actually not that far removed from what IStackableConfig.pop does, AFAICT.

> > How big is chunks?  If it's not too big, it might be easier (both in coding
> and in debugging) to just assertIn that 'daemon ready' occurs in
> '\n'.join(chunks).
> 
> In this case I want to be sure that that message isn't split between
> chunks, because other code depends on it - so this is testing the
> desired contract.

I understand that; hence the '\n'.  That does not occur in the string you're looking for, and so will reveal any splitting that may happen inside the string.  I think the error output from a failure would be very helpful, since it would give the entire relevant text (with the chunk boundaries clearly visible).

> > Also, there's a small problem with indentation in these situations: the
> continuation of the "heading" of the code block (in this case the nested()) is
> indented at the same level as the body of the code block, with nothing
> inbetween.
> >
> > The review team didn't like that, so we came up with two alternative
> suggestions for getting around it:
> >
> > 1. Keep whatever you need for the "heading" in variables so it fits on one
> line.
> >
> > 2. Separate the "heading" and the block's body with a comment.
> 
> Whats the motivation here? Its easily readable, visually distinctive
> (the ): line is a clear marker).

We aim for absolutely minimal overhead to reading and understanding the code.  Think Kernighan's Law and barrier to entry, but also simply "drag."  From that perspective, it was considered helpful for the reader to see the structure of the code clearly from a quick scan along the left-hand margin without (or before) mentally parsing the code.

(One of the main functions of our reviews is to ingrain and maintain consistent habits that serve these goals.  It's actually this exact quality in our programming that keeps me happy to work on this codebase.)

> Ohm and the 
>  ("""\
>   stuff
>
> is just how dedent is used, because it strips leading match spaces putting stuff on the first line is fail.

Ah yes, that is a bit of real nastiness in dedent.  Personally I fail to see why it's not a bug.

I hate to say it, but I think it's still better to solve it with lstrip('\n'), in order to eliminate any shadow of a doubt for a future reader that there may be invisible-but-significant whitespace in the source code.  You may be careful enough not to insert any, but the reader won't know who last edited the line.

Jeroen

« Back to merge proposal