yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

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.