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
Description: PGP signature
_______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork