Launchpad itself

Code review comment for lp:~mbp/launchpad/612171-diff-generation-spam

612171-diff-generation-spam
Merge into devel

Revision history for this message

Martin Pool (mbp) wrote on 2011-10-31:

> This looks very nice. Since it fixes a bug, it should provide a test to
> ensure the bug stays fixed, i.e. that a pending write does not generate an
> emeasily ail.
>
> Was providing retry_error_types as a list rather than a tuple deliberate?
> Given that the main difference between lists and tuples is that lists are
> mutable, are you intending the class variable to be mutated? Since that would
> affect all instances of the class, I think that could be confusing.

It was intentional. I think there is a semantic difference which is that lists convey there can be any arbitrary number of elements and all the elements are treated identically, which is the case here. I think tuples are generally more important when the structure is more like a mathematical vector with different positions having different meanings or dimensions. I don't feel that getting protection against "accidental" mutation is important here because there are so many other ways that can be broken in Python, most obviously by just assigning to the attribute on either the instance or the class.

But, I don't really care, if Launchpad style or consistency wants a tuple I'll change it.

> We care about pending writes because we want the diffs to be in sync with
> information displayed on the branch page.
>
> If we generate a diff using the last_scanned_id, the diff will be soon be
> stale, and if the user deleted the relevant revision, the diff will fail to
> generate. We would also want to schedule a new diff to be generated once the
> scan completes.
>
> If we generate a diff using an un-scanned last_mirrored id, it will be newer
> than the other information we display about the branch, and Launchpad could
> get upset that we ask it to display information about the revision, when the
> revision hasn't been scanned yet.

OK, thanks for explaining that. That confirms Launchpad should just cope with it, certainly without complaining to the user. It seems like we can get quite similar cases if the user just pushes immediately after the diff job starts, rather than just before.

> This looks very nice.  Since it fixes a bug, it should provide a test to
> ensure the bug stays fixed, i.e. that a pending write does not generate an
> emeasily ail.
> 
> Was providing retry_error_types as a list rather than a tuple deliberate?
> Given that the main difference between lists and tuples is that lists are
> mutable, are you intending the class variable to be mutated?  Since that would
> affect all instances of the class, I think that could be confusing.

It was intentional.  I think there is a semantic difference which is that lists convey there can be any arbitrary number of elements and all the elements are treated identically, which is the case here.  I think tuples are generally more important when the structure is more like a mathematical vector with different positions having different meanings or dimensions.  I don't feel that getting protection against "accidental" mutation is important here because there are so many other ways that can be broken in Python, most obviously by just assigning to the attribute on either the instance or the class.

But, I don't really care, if Launchpad style or consistency wants a tuple I'll change it.

> We care about pending writes because we want the diffs to be in sync with
> information displayed on the branch page.
> 
> If we generate a diff using the last_scanned_id, the diff will be soon be
> stale, and if the user deleted the relevant revision, the diff will fail to
> generate.  We would also want to schedule a new diff to be generated once the
> scan completes.
> 
> If we generate a diff using an un-scanned last_mirrored id, it will be newer
> than the other information we display about the branch, and Launchpad could
> get upset that we ask it to display information about the revision, when the
> revision hasn't been scanned yet.

OK, thanks for explaining that.  That confirms Launchpad should just cope with it, certainly without complaining to the user.  It seems like we can get quite similar cases if the user just pushes immediately after the diff job starts, rather than just before.

« Back to merge proposal