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