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 <d...@axtens.net>
Thanks for your persistence!
Regards,
Daniel
signature.asc
Description: PGP signature
_______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
