=== Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk [08:04] anyone around ? [08:04] https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011 [08:46] lifeless: Are you that busy that you need to work on a Sunday? [08:54] StevenK: I had a very fragmented week last week [08:55] the first few days of caffeine detox were a bitch [08:55] + moved house [08:55] + this has been annoying me since the epic [08:56] StevenK: are you a reviewer yet ? [08:57] jelmer: hai [08:57] lifeless: Mentored by thumper, and I'm about to go out [09:01] have fun === lifeless changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:04] lifeless, hi === Ursinha-afk is now known as Ursinha [18:39] jelmer: hi [18:39] lifeless, 'morning [18:42] jelmer: are you a full reviewer now ? [18:42] lifeless: yep, have been for some time [18:42] care to do a small review ? https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011 [18:43] lifeless: Sure [18:43] I need to get some groceries before the shops close, but I'll have a look when I get back. [18:44] thanks! [22:24] thumper: can I get a review ? [22:25] lifeless: I'm sprinting with wallyworld_ this week, so my time to response may be a little lower than normal [22:25] so almost glacial [22:26] ah [22:26] see, I have this patch [22:26] that removes an O(N^2) behaviour in the bug attachments API [22:26] its been our biggest oopser since the epic [22:26] I'm excited. [22:27] I'll see if mwhudson has a cycle or three [22:27] mwhudson: ^ [22:27] well [22:28] noone is around from linaro on mondays to see me slacking on that front... [22:28] its small [22:28] link me up [22:28] https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011 [22:30] the typo on line 173 i'm fixing now [22:31] hmm [22:31] the bug xyz text on the merge proposal links to https://code.edge.launchpad.net/bugs/625102 :( [22:31] <_mup_> Bug #625102: exported_as does not handle overriding an unexported attribute [22:32] yes [22:32] it made this more awkward than I would have liked. [22:33] If there is another alternative workaround I'm happy to do-over - ec2 test told me some stuff failed, and I've fixed and checked all that locally, so I'm pretty sure there are no dragons left [22:33] no, the rootsite of the url is wrong [22:33] that's what i was complaining about [22:33] oh, ah! [22:33] it works :P [22:34] yeah [22:34] i don't know what i think about that aspect :) [22:41] what do you think about the other aspects [22:42] ah sorry got distracted by email [22:45] * lifeless undistracts mwhudson [22:51] lifeless: i think you need to have a big XXX block explaining what attachments_unpopulated is and how it differs from plain attachements [22:52] mwhudson: like the one in the docstrings on the two properties? [22:54] yeah, that'd be good i think [22:54] er [22:54] no [22:54] in the interface [22:55] this poses a quandry [22:55] I don't want to repeat myself. [22:55] The implementation is the right place to write this up - which I've done. [22:55] the shenanigans involved are certainly totally irrelevant to the API [22:56] sure [22:56] and at the interface level these two things are identical - to the extent that the interface models behaviour [22:56] so maybe i should name the problem rather than a solution: [22:56] if someone looks at the interface as it is in your branch, they are going to say "wtf" [22:57] I'm happy to add a 'see lp.bugs.model.bug.Bug.attachments' [22:57] but having the implementation and explanation separated pretty much guarantees skew in the future. [22:58] + # See lp.bugs.model.bug.Bug.attachments for why there are two similar [22:58] + # properties here. [22:58] you should also link to the lazr.restful bug [22:58] attachments_unpopulated = CollectionField( [22:58] lifeless: that'll do [22:58] well [22:58] the lazr.restful bug wouldn't eliminate having two properties [22:59] it would however let the obvious one be the less-eager-loading one as we did in Person.allmembers and Person.all_members_prepopulated [22:59] .sure [22:59] it should still be mentioned somewhere [23:00] Unless we're going to actually come back and change this when the bug is fixed in lazr.restful, I don't see much point. [23:00] i assumed we were [23:00] its cosmetic [23:01] having most of the code in launchpad reference this peculiarly named property is tech debt, surely? [23:01] it would be a little nicer, but not faster or more correct, if lazr.restful had allowed exporting it [23:01] well sure it's cosmetic [23:01] cosmetics are important [23:01] mwhudson: it being a property is tech debt [23:01] uhm [23:02] I can mention it, I just don't see the value in doing so. [23:03] lifeless: the way things are, i think it's very likely that the next time someone writes code that iterates over attachments, they will say [23:03] for attachment in self.bug.attachements: ... [23:03] because that's the natural thing to write [23:04] wouldn't it be better if that was actually the correct thing to say? [23:04] it would be, I agree. there are - maybe - 5 bugs needed to fix to make that correct. [23:04] we need eager loading [23:05] we need a QL of some sort to control it [23:05] we need separated domain and storage code [23:05] we need the API to use object introspection on interfaces to specify a QL that will eager load appropriately [23:06] look, I'll reference the bug, I just think its minutiae; we could fix all the XXX linked bugs and not be hugely better off. [23:06] or we can fix the structural things, and most of the XXX comments will just get deleted as we swing past the code and overhaul it. [23:08] maybe i'm too pessimistic, but launchpad is littered with 'temporary fixes' from 2005 [23:08] i know you're good at fixing that sort of problem [23:08] absolutely. [23:08] (to both lines :P) [23:08] + # attachments_unpopulated would more naturally be attachments, and [23:08] + # attachments be attachments_prepopulated, but lazr.resful cannot [23:08] + # export over a non-exported attribute in an interface. [23:08] + # https://bugs.launchpad.net/lazr.restful/+bug/625102 [23:08] <_mup_> Bug #625102: exported_as does not handle overriding an unexported attribute [23:08] attachments_unpopulated = CollectionField( [23:08] lifeless: thank you [23:08] All I'm saying is that the tech debt lets-use XXX comments approach seems to be not working. [23:09] lifeless: it has worked out ok a few times for me [23:09] because, the deep problems aren't actually being tackled, or something. [23:09] or aren't getting enough official airtime. I don't have enough data to isolate causes yet. [23:09] when i've worked around a bug in bzr, i've been able to grep for the bug number when the fix in bzr has been released [23:09] (and twisted) [23:10] generally though thats not something thats really a workaround to a structural issue in LP is it ? [23:10] like, having to do X to bzr, which it doesn't support, because Y is brain-damaged in LP itself [23:11] i guess not [23:12] I think things that can't be done the right way are worth noting with bugs; but the right bug number should be the root cause. [23:12] anyhow, thats committed and pushed. [23:15] lifeless: how did you find the places to s/attachments/attachments_unpopulated/, just grep? [23:16] mwhudson: yes [23:16] must have been fun :-) [23:16] not at all [23:16] it will work if the wrong one is used, just be more expensive. [23:16] I don't *think* I missed any [23:20] lifeless: i don't find the docstring on BugAttachment.message very enlightening [23:21] so, you understand what its doing ? [23:21] i do now [23:21] any suggestions for what to add into it ? [23:21] trying to come up with some :-) [23:22] maybe something like "see lp.bugs.model.bug.Bug.attachments for where the IIndexedMessage is injected" [23:23] and maybe change the first line to "this is a cached property to allow message to be prepopulated" [23:23] - """This is a property to allow message to be an IIndexedMessage. [23:23] + """This is a cachedproperty to allow message to be an IIndexedMessage. [23:24] [23:24] - This is needed for the bug/attachments API calls, which seem to be the [23:24] - only users of .message. [23:24] + This is needed for the bug/attachments API call which needs to index [23:24] + an IIndexedMessage rather than a simple DB model IMessage. See [23:24] + Bug.attachments where the injection occurs. [23:24] lifeless: +1, thanks [23:25] lifeless: no other comments, apart from "why is there SAMPLE_PERSON_EMAIL and USER_EMAIL??" [23:25] but that can rest for another day [23:25] if I knew [23:25] having found they were the same, I thought I'd make it clear they were the same :) [23:25] yeah fair enough [23:25] * mwhudson goes to do things on the website [23:25] thanks