Review: Approve
Hi Francesco. Thank you for this good work.
I am conditionally approving this, with the changes I note below. If you
disagree with my requests, that's fine; we can just work it out when our work
hours overlap.
- line 88 of the diff has an XXX that we need to address ("XXX frankban
2011-12-16 further investigation needed"). We need to either address and
remove the XXX or follow the XXX policy and give it a bug (see
https://dev.launchpad.net/PolicyandProcess/XXXPolicy). You said that we could
not address the XXX without significant refactoring; and my further
understanding was that we did not need the XXX behavior for our goals.
Therefore we pursued what would be necessary to follow the XXX policy. I would
like to see the following:
* You file a bug for adding support to exclude conjoined bug tasks from
milestone tag searches.
* You add that bug number to the XXX, per the policy, with a comment that
states the problem to be solved (not the fact that it needs more investigation).
* You change the ValueError check ("if (params.exclude_conjoined_tasks and
not (params.milestone or params.milestone_tag)):") to blow up if someone tries
to exclude conjoined tasks with a milestone tag, so that it is clear that it is
not supported. A better error for this particular case might be
NotImplementedError. You add an XXX to this error with the same bug number,
clarifying that it can be removed when the bug is resolved.
* For this to work, we'll need to change the call in
getPrecachedNonConjoinedBugTasks to only ask for excluding non-conjoined bug
tasks when it is possible. This again would be an XXX with the same number.
Alternatively, you could change whatever code uses that function to use a
different one, I suppose.
I can see an argument for what you have done, which pretends to support
excluding the conjoined bug tasks but does not. I really think being clear
about it is better. Solving it would be better still, of course. I'd be happy
to review the concerns about performing the refactoring if you think that might
have a chance of helping; however, I'm also fine trusting the analysis that you
and Brad performed.
- You have used tabs instead of spaces, such as in the zcml files (see lines
278 and 279 of the current diff). Please switch to spaces only.
- We have functionality and code to handle user metadata on metadata tags. I
think we should probably have a few tests of that.
Lastly, here are comments, notes and suggestions.
- The tests are good. Thank you.
- Our style guide states that our comments should be full sentences.
Therefore, "# Circular reference prevention." would be better written as "#
Prevent circular references." I don't require this change, but it would be
nice.
- I suggest adding comments to getTags and setTags indicating that the tags are
not a property because of the "user" argument to setTags, and the Launchpad
desire to avoid model code that gets the user from the request or the
"launchbag" thread locals.
- On IRC we discussed whether you needed to move milestone_tags to the end of
the argument list of BugTaskSearchParams's __init__, for backwards
compatibility for code that used placeful arguments. We agreed that we would
rely on tests to show us that this was unnecessary.
- The MilestoneSpecificationTest and MilestoneBugTaskTest could maybe have some
stuff abstracted. You probably noticed as well. Don't bother doing it, unless
you really want to.
- Thank you for the spelling and whitespace fixes. I like doing this kind of
thing as well. People point out to me, though, that these are the kinds of
things that can easily be pushed out to a separate branch for review, not
muddying an existing branch and keeping the line count low. My
counterargument, at least for changes like the ones you have made here, is that
the cost of making another branch is higher than the cost of the reviewer
skimming over spelling changes. You should keep this tension in mind at the
least.
--
https://code.launchpad.net/~frankban/launchpad/bug-904335-get-tags/+merge/87489
Your team Launchpad Yellow Squad is subscribed to branch
lp:~yellow/launchpad/bug-904335-devel-base.
--
Mailing list: https://launchpad.net/~yellow
Post to : [email protected]
Unsubscribe : https://launchpad.net/~yellow
More help : https://help.launchpad.net/ListHelp
