On 2016-10-25 09:27, Daniel Axtens wrote:
Hi Stephen,
I think we're down to the final code-review details - hopefully no more
architectural or design issues!
Here goes:
v4:
- Convert 'SeriesRevision'-'Patch' relationship from one-to-many to
many-to-many
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.
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 :)
class CoverLetter(Submission):
- pass
+
+ @property
+ def series(self):
+ """Get a simple series reference.
+
+ Return the last series revision that (ordered by date) that
+ this submission is a member of.
+
+ .. 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?
+ """
+ # 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 :)
- 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.
+ 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()
+ @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.
+ return self.total == self.actual_total
+ # 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?
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.
Stephen
_______________________________________________
Patchwork mailing list
[email protected]
https://lists.ozlabs.org/listinfo/patchwork
