[PATCH v6 2/8] models: Add 'SeriesRevision' model

[Daniel Axtens]

Hi Stephen,

>> I missed this when it first went in. My understanding is that this is 
>> to
>> allow partial re-spins: that is:
>> 
>> v1 0/3 blah-cover
>> v1 1/3 blah 1
>> v1 2/3 blah 2
>>   v2 v2/3 blah-blah 2
>> v1 3/3 blah 3
>> 
>> Is that right?
>
> Yup, that's the idea. We don't use this yet, but we will in the future. 
> I figured it was best to set up the models to allow us do this in the 
> future, rather than having to do a migration down the line. I'm treating 
> versioning as a separate, bonus feature.

Sounds good.

>> So you'd have:
>>  SeriesRevision v1 blah-cover containing v1 1/3, v_1_ 2/3, v1 3/3
>>  SeriesRevision v2 blah-cover containing v1 1/3, v_2_ 2/3, v1 3/3
>> 
>> I think that makes sense, just checking if that's what you
>> intended. (And if so, do we have a test for it?)
>
> Yup, that's the intention but I don't have tests as the actual 
> functionality isn't implemented yet. The best I could do is test that 
> the ManyToMany field works, but I think it's safe to assume it does :)

No worries.

>>> +        .. warning::
>>> +          Be judicious in your use of this. For example, do not use 
>>> it
>>> +          in list templates as doing so will result in a new query 
>>> for
>>> +          each item in the list.
>> 
>> Apologies if this has been asked before: but this can't be fixed with a
>> select_related or a prefetch_related?
>
> Not that I could see. It's not an issue anyway, once we just don't call 
> it from said views (which we don't). Maybe I could just remove the 
> warning?

I'd probably remove the comment, but I'm not particularly fussy. I just
wanted to check the details.

>>> +        """
>>> +        # NOTE(stephenfin): We don't use 'latest()' here, as this can 
>>> raise an
>>> +        # exception if no series revisions exist
>>> +        return self.series_revisions.order_by('-date').first()
>>> 
>> You seem to have two copies of this function, one in CoverLetter, and
>> one in Patch. Is there any way to remove the duplication? I haven't dug
>> deep into the class structure here, but could it be pushed to a
>> super-class?
>
> Yeah, Andy suggested the same. We can't do this, however, as 
> 'series_revisions' isn't a valid field for 'Submission'. I figure 
> duplicated code is better than wrong code :)

Okay, no worries. I agree wrong code is worse than duplicated code :)

>>> -    def save(self, *args, **kwargs):
>>> -        if not hasattr(self, 'state') or not self.state:
>>> -            self.state = get_default_initial_patch_state()
>>> -
>>> -        if self.hash is None and self.diff is not None:
>>> -            self.hash = self.hash_diff(self.diff).hexdigest()
>>> -
>>> -        super(Patch, self).save(**kwargs)
>>> -
>>> -        self.refresh_tag_counts()
>>> -
>> 
>> It looks like you're just moving the save method here and below.
>>  - Is there any functionality change?
>>  - Is there any reason to move it?
>
> Nope - no functionality change. Looks like a hangover from an earlier 
> revision. I'll remove this hunk.

Thanks.

>>> +    @property
>>> +    def actual_total(self):
>>> +        return self.patches.count()
>> 
>> I'm not a fan of the name actual_total here. Could we use
>> received_total? Otherwise it's not clear to me what the distinction
>> between 'actual' and non-'actual' is.
>
> Sounds good to me.
>
>>> +    @property
>>> +    def complete(self):
>> 
>> And here - completely_received?
>
> Yup.

Thanks.

>>> +        # don't override user-defined names
>>> +        if not self.name:
>>> +            self.name = _format_name(cover)
>>> +        # ...but give cover letter-based names precedence over 
>>> patch-based
>>> +        # names
>>> +        else:
>>> +            try:
>>> +                name = SeriesRevisionPatch.objects.get(revision=self,
>>> +                                                       
>>> number=1).patch.name
>>> +            except SeriesRevisionPatch.DoesNotExist:
>>> +                name = None
>>> +
>>> +            if self.name == name:
>>> +                self.name = _format_name(cover)
>> 
>> Have I got this right?
>> If there is no recorded name, use the name from the cover letter
>> Otherwise, if there _is_ a recorded name:
>>  - let name be the first patch name or None
>>  - if that is the name we have, set the name to be the name from the
>>    cover letter.
>> 
>> So, what I think you're doing is using the following priority list:
>> 1) User provided name
>> 2) Cover letter name
>> 3) First patch name
>
> Yes, exactly.
>
>
>> And then you're trying to make sure stuff is updated - if you start 
>> with
>> a patch and then get a cover letter you upgrade to the cover letter 
>> name.
>
> Yup. We allow "upgrading" of the name, i.e. cover letter name -> 
> user-provided name, and never "downgrading".
>
>> Is that what you're trying to do? Could you clarify the comment? (And
>> maybe restructure the code?)
>
> Yeah, I'll expand on the comments. Any suggestions for how the code 
> could/should be reworked?

Thanks. I'm not really sure how to rework the code - I tried while I was
writing my email but I didn't come up with anything better. A clarified
comment would be sufficient.

>> Anyway, I think we're nearly there!
>
> Hopefully :) I'll wait for reviews for the rest of the series before 
> sending another revision, if that's ok by you.

That's a good call.

With the changes as agreed, feel free to put the following on your next
revision:
Reviewed-by: Daniel Axtens <dja at axtens.net>

Thanks for your persistence!

Regards,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 859 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20161026/c019b4aa/attachment-0001.sig>