=== jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:17] 'morning jtv [09:17] Good morning jelmer_! How's the weather over there? [09:20] jtv: It's nice and sunny, nothing to complain :-) [09:20] jelmer_: good, because I'll be there in a few weeks. [09:26] ah, nice :-) [09:26] jtv: Mind if I add a branch to the queue? [09:27] Ahhh, I was wondering why you were all friendly this morning. :-) Go ahead. [09:28] hehe, thanks === jelmer_ changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue: [bryce, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue: [bryce, jelmer, gmb(http://bit.ly/bkJ7dM)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue: [jelmer, gmb(http://bit.ly/bkJ7dM)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [10:01] jelmer_: looks good... one small thing: consider using "print" for values in doctests so the test doesn't fail if 'value' turns up as u'value' etc. [10:05] jtv: ok [10:05] jtv: and thanks for the review :-) [10:07] np [11:15] Explosions. I may lose power. [11:34] jtv: :-/ [11:34] Are the riots still ongoing? I didn't see much in the media recently so I assumed it had quieted down. [11:44] noodles775: let's see if we can sort this out quickly ... ;-) [11:44] Great :) [11:44] If talking's easier, we can mumble too. [11:51] jelmer_: no, that's all over. The army/government has promised they will consider holding elections in the further future (for now they say they won't because people won't vote for them yet), and in the local way, everything looks like it's gone completely back to normal. A few buildings are gone, but that's not entirely abnormal in peacetime. === jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [11:53] jtv: ah, good :-) [11:54] jelmer_: in a way... but nothing's been solved, the army still controls the media, 69% of money in the banks still belongs to under 1% of the population, and the powerful can still kill normal people in large numbers without ever being called to task. [11:55] henninge: https://dev.launchpad.net/StormMigrationGuide#Prejoins [11:55] jelmer_: anyway, I'll try to keep the politics out of this channel. :) [11:56] :-) [12:00] gmb: nitpick about existing code that you only moved... reason = reason + x => reason += x [12:00] in _addReason. [12:02] gmb, are you here? [12:20] jtv, I am now [12:21] gmb: hi... quite a little jungle of similar text composition functions you moved there. [12:21] gmb: several questions about that code, even though I know it's pre-existing: what about the supervisor for a duplicate bug? I'm not sure but I think I didn't see that among the possibilities. [12:22] jtv, I'm going to fix those things in a subsequent branch. [12:23] (There are a lot of things missing, but we need to move the code before we start fixing the bugs, otherwise we'll just end up in conflict central) [12:23] Ah great. I was thinking, why not separate the "you are" / "you are in team %s which is" choice from the "subscribed to" / "bug supervisor for" choice and the "bug %s" / "a duplicate of bug %s" part [12:23] jtv, Yes, I think that would be a good way to go; we can subclass RecipientReason for handling those cases, which is something else I plan to do. [12:24] Thanks for placing it into contet. [12:24] s/contet/context/ [12:24] np [12:25] This is something you could have mentioned while asking "hey Jeroen, can I chuck this on the queue?" :-) [12:25] jtv, Mea culpa, duly noted. [12:25] (More to the point: should've gone in my MP) [12:25] ...and now you're depriving my of my chance to give out a Stern Look. [12:26] Oh well. [12:26] Heh. [12:26] But given all that, sure, the branch is good. [12:26] Thanks [12:28] gmb: I didn't care much for the "you are directly subscribed" text either... in real life we say "you are subscribed" and only qualify it when things get more subtle. [12:32] Agreed === jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === noodles775 changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: noodles_for_30 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:41] noodles775: sounds good... [12:41] hang on though; see if I can rustle up some action for that failed build we have. [12:41] I don't actually have an mp yet... just wanted the $30 :) === noodles775 changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:42] noodles775: well there's the catch... that money's supposed to flow the other way === henninge_ is now known as henninge [14:53] jelmer_ or jtv: if either of you have time, could you please take a look at https://code.edge.launchpad.net/~michael.nelson/launchpad/594492-present-bfjs-in-builder-history/+merge/27717 === noodles775 changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:54] Coming... [14:54] oh, cool stuff there... === jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:59] Thanks jtv! [15:00] Thank me when it's done. I can be mean. :) [15:00] Well, I always learn lots :) === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: jtv, Edwin || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:04] noodles775: you describe IBuildFarmJobSet as a set of package builds. That's not right, is it? [15:04] ajmitch, you may want to withdraw https://code.launchpad.net/~ajmitch/launchpad/fakesyncs/+merge/27615 if you don't feel it is the right approach. To do so, go to the 'Status:' field at the top and switch it to 'Reject'. [15:04] jtv: no, updating now. [15:06] noodles775: much less stringent, but in case you like it: you can describe :param status: If given, limit the search to builds with this status. [15:06] Or something like that. [15:07] jtv: doing so now. [15:08] jtv: also adding a :param user: [15:09] noodles775: binary_only sounds like it wants to be generalized... something like an optional "get them from this set" argument to override IBuildFarmJobSet with IBinaryPackageBuildSet. But I'm sure it does what's needed right now, so take that as a mere sidenote. [15:10] Noted :) [15:12] Then, about line 87 of the diff: kudos for checking, but I wonder if that shouldn't be an assertion. The way it looks to me, if somewhere in the caller there's a codepath that might lead to a nonsensical combination of arguments based on user input or whatever, then the caller broke the contract with this method by not filtering the input better. [15:15] jtv: The reason I used an exception was that it could be an API call. [15:15] noodles775: oic [15:17] noodles775: moving right along to line 138 of the diff, thanks to Stuart's wonderful work you can now do [15:17] store = IStore(BuildFarmJob) [15:17] By the way, please put those imports _all the way_ at the top of the method. [15:18] Like the way you're using find on a result! [15:19] Yeah, I just saw that we can now do that... (I'd not realised we had a version of storm that enabled chaining). [15:19] Although seeing it in the wild like this, I'm not sure I find it as clear as collecting a list of merge criteria and then passing them as find(MyClass, *criteria). Maybe I just need to get used to it. [15:20] With line 138... erm, how does that work? It just chooses the default flavour based on the model? [15:20] * noodles775 reads the db-policy again. [15:20] noodles775: yes, IStore uses DEFAULT_FLAVOR. There are also IMasterStore and ISlaveStore if that's not what you want. [15:21] noodles775: the elif at line 170 has two things wrong with it—the indentation of the comment above it, and a missing else. [15:22] Nice. Actually, I'd assume I can use slave here anyway. [15:22] * noodles775 looks [15:22] The indentation is just because the comment should be below right? [15:22] And the missing else? even if it should do nothing? else: pass? [15:24] Yes, probably with a comment. You may like rearranging them as well: [15:24] if user is None: [15:24] # Strangers don't get to see our privates. [15:24] ... [15:24] OK [15:24] elif user.inTeam(getUtility(ILaunchpadCelebrities).admin): [15:24] # Admins see everything. [15:24] pass [15:24] else: [15:25] Yep, that's clearer. [15:25] # Everyone else gets to see public stuff and what they own. [15:26] Maybe Storm will let you say something simpler for the Or on private being null or false. [15:26] In SQL I'd say "is not true," but in Storm Coalesce(Archive.private, False) would work as well. [15:26] Oh, that doesn't actually help with that last query does it? [15:27] No, it's in a separate () (in sql-terms). [15:27] But that's good to know about Coalesce [15:27] BTW beware that this query structure may well be the worst recurring performance hog in LP. [15:27] Yeah, I had two different methods, this one (left join) and using difference... do you know which would perform better? [15:28] (or did you mean specifically these OR clauses...) [15:28] I was thinking of a union: publicly visible items, and private ones that the user has a special right to. [15:28] Yes, that's exactly what I meant. [15:28] That's a good idea... I was working with a difference... I'll try that. [15:29] jtv: erm, actually, it's the "publicly visible items" that is the problem. [15:29] The recurring problem is that with the OR, you're combining two very different "shapes" of query: [15:29] Which is why I was using difference. [15:29] Oh, how are they the problem? [15:30] I mean I can see that there are just going to be _more_ of them, but what else am I missing? [15:30] To determine all the public ones requires the leftjoin on PackageBuild (afaics) [15:31] Ah, but you mean that bit is ok... it's more the additional team subquery etc.? [15:31] Yes. It makes you squeeze two different shapes of query into one. [15:31] One is a "narrow" one without the extra query, but very "deep" with lots of rows. [15:32] The other is a "wide" one, but with very few rows so still cheap overall. [15:32] With the "or," those two are flowing all the way through the same query plan. [15:32] Whereas doing a separate query for all the private ones I can see and unioning would solve that right? [15:32] Right. [15:32] I see. Great! [15:33] * noodles775 is glad there happened to be a db-expert on ocr :) [15:33] * jtv coughs modestly [15:36] * noodles775 wonders why he didn't notice the long line at 183 in the lint. [15:39] Maybe you added the Desc later? [15:39] Perhaps. [15:39] Anyway, fixed. [15:40] Cool. [15:40] Ready to move on? [15:40] Well, I haven't re-written the query yet, but I'll do that later so yes :) [15:41] OK! I'm looking at the test setup now, ll. 232 and down. [15:41] (actually getting strange import error atm: You should not import IRepresentationCache from lazr.restful.interfaces [15:41] Yep. [15:41] Maybe check the __all__ in that lazr file? === matsubara is now known as matsubara-lunch [15:42] noodles775: Oh, then maybe it should be lazr.restful.interfaces.some_module [15:43] The traceback doesn't mention any code that I've touched, but I'll look at it later... the test setup? [15:45] No idea, sorry. [15:45] No, I meant, getting back on topic, you said: I'm looking at the test setup now, ll. 232 and down. [15:45] Oh ah! [15:46] Right—that setUp method looks pretty unwieldy, but when I look closer I see it's only about 5 variables being set up. The rest is self.build_farm_jobs.append lines. [15:47] So I was thinking: [15:47] (i) move the creation of that list to a helper method and then self.build_farm_jobs = self._makeThatList(), and [15:47] (ii) maybe just write the list in one go instead of with append() lines. [15:48] OK. [15:49] Because lots of text in a setUp often makes it hard to extend or fix tests. [15:49] Sounds good. [15:52] Oh, another point there (and sorry for this, since you're not causing it only revealing it in your diff): I wouldn't derive a base test class from TestCaseWithFactory and then derive the actual tests from that. Asking for trouble. Better to create a mixin, and have the concrete test classes derive from TestCaseWithFactory and the mixin. Python has no qualms about letting you use stuff that isn't in your class but will be in a d [15:54] Right, I'll update it (and I probably was the cause). [15:54] Strangely, the import error I was seeing was fixed by a make clean;make. [15:55] Actually, that makes sense, since I'd since merged devel and not re-run make. [15:56] "the world will never know! [15:56] " [15:57] The makeBuildFarmJob method is shaping up nicely... maybe it's at a point where it's worth moving to the factory? ISTR we already have something there, in which case a merger may be possible. [15:58] Heh... I had this discussion earlier with a different branch. A BuildFarmJob should never be instantiated on its own, so I don't think we should include it in the factory. We should always instead be instantiating the leaf objects (BinaryPackageBuild, SPRecipeBuild). [15:58] It is tested here internally, but all other callsites should not ever be creating a BFJ... and IBFJ.specific_job will raise an appropriate exception to tell you that. [15:59] OK [15:59] I've explicitly added a comment in test_packagebuild, and probably should do so here too. [16:00] Sure... could be brief afaic [16:03] noodles775: in line 266 of the diff, I think testing against self.build_farm_jobs[:2] is clear enough. How about: [16:03] * holding the result of getBuildsForBuilder in a variable, [16:04] Yep. [16:04] * comparing its contents to [job for job in self.build_farm_jobs if job.status == BuildStatus.FULLYBUILD] [16:04] * and asserting that that is a nonempty, proper subset of self.build_farm_jobs. [16:04] OK. [16:06] Actually this style of testing is a little dangerous in that it invites the old Soyuz problems back. You're implicitly testing different scenarios by making sure they're all represented in the data, then running the code on it onces. [16:07] I just thought it was better than creating the data for each test separately? [16:07] Well "better" can be a difficult thing to estimate when there are multiple variables. :-) [16:08] And I don't see why you say "implicitly testing", when each case is tested explicitly? [16:08] But if you want to test "this filters by status," you can create a list with just 2 BFJs, one with the status you're looking for and one with another one. [16:09] It's implicit in that you're not testing against a set that pinpoints the property that matters, so you can say "baby A had a dry bottom and baby B was wet after the test." [16:10] Instead, you're implementing a research programme: "of all the babies we tested, the ones using brand X were dry and the ones using Y were wet." [16:11] That can be very effective at finding unexpected changes. It can also be rigid and brittle. [16:12] Sorry, I don't see how the tests are doing that. For example, if test_getBuildsForBuilder_by_status was updated to show the result only contained items with the expected status, and then demonstrated with a different status. [16:13] But you're relying on both properties being represented in a generic sample set, rather than on a minimal one. [16:15] Yes, because I don't see why a minimal one is less prone to false testing? (ie. implementation filters the correct single instance but for the wrong reason). [16:15] That's where the ceteris paribus principle comes in. [16:15] But I can remove the code from the setup and put it in the individual tests using minimal data. [16:16] *ugh* latin. [16:16] :-) [16:16] It means, "all other things being equal." [16:16] You can create two objects that are clearly identical in all respects but one, and then show that that one difference is what determines the outcome. [16:17] If you ever have any particular reason to believe that some other difference might influence the outcome when it shouldn't, that's also something you can test against. [16:17] Yes, if that were the case it would be fine, but I didn't think we could do that (eg. the related archive name is random etc.) [16:17] Right... so, do it in a sensible way. OK. [16:18] I'll update the tests :) [16:18] Not trying to make a religious conversion here. Common sense still applies. :-) === matsubara-lunch is now known as matsubara [17:02] jtv: I've pushed the updated query, but will have to do the test updates first thing in the morning. Thanks for all your help. [17:03] noodles775: thanks for taking the abuse so well. :) Still have a few more lines to go if you're up for it. [17:03] Yep, I will be up for it tomorrow, if that's ok with you :) [17:04] (or if you want to add it to the MP... I was going to just include a url to this irc log, but you could do that and add any extra info on the MP?) === jtv changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [17:09] Sorry EdwinGrubbs, leaving you holding the bag. :) Been a busy day, review-wise. [17:10] jtv: bye. The queue is empty, so that's good. [17:11] yup. === jtv is now known as jtv-afk === salgado is now known as salgado-lunch === EdwinGrubbs is now known as Edwin-lunch === henninge changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [17:52] Edwin-lunch: Hi! When you are filled it would be great if you could look at my slightly over-sized branch. ;-) [17:55] Edwin-lunch, after you look at henninge's huge branch, i have a fun one [17:55] https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/shim-objects/+merge/27739 === leonardr changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue: [henninge, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [17:56] Oh yeah right, mine is here: https://code.edge.launchpad.net/~henninge/launchpad/bug-545354-enable-sharing/+merge/27123 [18:18] leonardr: I had all the fun and let Edwin-lunch have nothing ... ;) === henninge changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [18:18] well, at least he is having lunch. === henninge is now known as henninge-afk [18:19] wel, more like brb [18:28] henninge-afk: how about 'collection_of'? that's what we call it in lazr.restful === salgado-lunch is now known as salgado === sinzui changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue: [henninge, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === henninge-afk is now known as henninge [18:54] leonardr: perfect! ;) [19:13] Edwin-lunch: may i throw another on your queue? === bac changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: - || queue: [henninge, sinzui, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === Edwin-lunch is now known as EdwinGrubbs [19:32] henninge: I'm starting on your branch now. === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue: [leonardr, sinzui, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [19:33] EdwinGrubbs: thanks! [19:34] EdwinGrubbs: I may be afk for a little but will definitely come back later today. [20:19] EdwinGrubbs: Did you notice, that I am not merging into devel but into our "recife" integration branch? [20:23] henninge: yep, I noticed. [20:29] mars: sadly I can't reject my own merge proposals :) [20:30] ajmitch, ? [20:30] mars: you told me to reject the fakesyncs one, I only have the option of setting it to IP, needs review or merged [20:30] s/IP/WIP/ [20:32] is 'Rejected' greyed out? [20:32] no, it didn't show at all [20:32] ajmitch, I can set it to Rejected if you want [20:32] that is strange [20:33] probably due to team membership on the target branch or something like that [20:33] you may as well, I'd set it to work in progress [20:33] ok. You can always resubmit later [20:33] will do [20:34] sorry to waste your time with it [20:34] no problem. sometimes you find a better way to tackle something [20:34] so go for it :) [20:59] henninge: maybe you want to review my follow-up launchpadlib branch? https://code.edge.launchpad.net/~leonardr/launchpadlib/delay-http-requests/+merge/27759 [21:04] leonardr: r=me ;-) [21:07] huzzah === salgado is now known as salgado-afk [22:24] henninge: review sent [22:24] EdwinGrubbs: cheers [22:25] sinzui: I'm starting on your review now [22:26] \o/ === matsubara is now known as matsubara-afk [23:10] sinzui: is it really necessary to list enable_bug_expiration and remote_product in the View.field_names thus necessitating the ghost widgets? Couldn't you have instead: [23:10] def setupWidgets(): [23:11] self.widgets += self.widgets['bugtracker'].getAdditionalWidgets() [23:15] EdwinGrubbs, View will not handle the fields and certainly not apply the update() to the object unless we list them [23:17] EdwinGrubbs, I think the feature we want is to delegate rendering in the page. We do want the view to manage the data it is working with, and the widget is the device it uses to do validation. [23:19] sinzui: ugh, I just have one other comment. In the class declaration for BugTrackerVocabulary, you set _filter=1==1, why not _filter=True? [23:20] I like your suggestion [23:21] I was concerned about storm, but I should have tried it instead of applying an obvious hack === sinzui changed the topic of #launchpad-reviews to: on-call: Edwin || reviewing: henninge || queue: [leonardr, sinzui, bac, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [23:31] sinzui: "In a registered bug tracker" sounds funny, and it makes the user wonder what "registered" means. How about, "In an external bug tracker" [23:32] I was thinking that too. [23:32] I hesitated. Since your branch will be ready to add "Register a bug tracker" we should choose text that is consistent [23:33] sinzui: it would also be nice to add some spacing between the radio buttons, so you don't have to look back to the beginning of the line to tell if the next text field is really part of that selection. [23:33] I think I can do that via css [23:33] sinzui: I could make the link say "Register an external bug tracker" [23:34] Do we need "external" in that case [23:34] Would someone what to register Launchpad as a bug tracker? [23:35] sinzui: no, I was just thinking if you wanted "In an external bugtracker" and "Register a bug tracker" to sound more related. [23:37] sinzui: I've submitted the review. All the comments are what we discussed on IRC already. [23:37] There are a lot words on the page. I suspect we will land some text revision after user feed back next week because the options are too complex to scan [23:37] thanks [23:37] I can dedicate my time to it tomorrow === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue: [bac, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [23:48] gmb: ping [23:49] krkhan, It's ten to midnight; any chance it can wait until tomorrow? [23:51] gmb: sure. i'm not in a hurry. i just resubmitted my findattachments merge proposal. the new branch uses horspool algo for memory efficient file search. but it'll still need lots of discussion as i still think it'll require lots of changes before being lp ready :) i'll catch you tomorrow then [23:54] krkhan, Okay; can you request a review from BjornT as well please? He'll want to have some input on this. [23:54] krkhan, I'll try to take a look some time tomorrow. [23:56] gmb: okay. req'ed BjornT for a review as well. thanks [23:57] krkhan, Np. G'night