| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-afk | ||
| lifeless | anyone around ? | 08:04 |
|---|---|---|
| lifeless | https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011 | 08:04 |
| StevenK | lifeless: Are you that busy that you need to work on a Sunday? | 08:46 |
| lifeless | StevenK: I had a very fragmented week last week | 08:54 |
| 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:55 |
| lifeless | StevenK: are you a reviewer yet ? | 08:56 |
| lifeless | jelmer: hai | 08:57 |
| StevenK | lifeless: Mentored by thumper, and I'm about to go out | 08:57 |
| lifeless | have fun | 09:01 |
| === 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 | ||
| jelmer | lifeless, hi | 13:04 |
| === Ursinha-afk is now known as Ursinha | ||
| lifeless | jelmer: hi | 18:39 |
| jelmer | lifeless, 'morning | 18:39 |
| 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:42 |
| 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:43 |
| lifeless | thanks! | 18:44 |
| lifeless | thumper: can I get a review ? | 22:24 |
| 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:25 |
| 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:26 |
| lifeless | I'll see if mwhudson has a cycle or three | 22:27 |
| lifeless | mwhudson: ^ | 22:27 |
| mwhudson | well | 22:27 |
| 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:28 |
| lifeless | the typo on line 173 i'm fixing now | 22:30 |
| 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:31 |
| lifeless | yes | 22:32 |
| lifeless | it made this more awkward than I would have liked. | 22:32 |
| 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:33 |
| mwhudson | yeah | 22:34 |
| mwhudson | i don't know what i think about that aspect :) | 22:34 |
| lifeless | what do you think about the other aspects | 22:41 |
| mwhudson | ah sorry got distracted by email | 22:42 |
| * lifeless undistracts mwhudson | 22:45 | |
| 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:51 |
| lifeless | mwhudson: like the one in the docstrings on the two properties? | 22:52 |
| mwhudson | yeah, that'd be good i think | 22:54 |
| mwhudson | er | 22:54 |
| mwhudson | no | 22:54 |
| mwhudson | in the interface | 22:54 |
| 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:55 |
| 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:56 |
| 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:57 |
| 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:58 |
| 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 | 22:59 |
| 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:00 |
| 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:01 |
| lifeless | I can mention it, I just don't see the value in doing so. | 23:02 |
| 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:03 |
| 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:04 |
| 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:05 |
| 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:06 |
| 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:08 |
| 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:09 |
| 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:10 |
| mwhudson | i guess not | 23:11 |
| 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:12 |
| mwhudson | lifeless: how did you find the places to s/attachments/attachments_unpopulated/, just grep? | 23:15 |
| 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:16 |
| mwhudson | lifeless: i don't find the docstring on BugAttachment.message very enlightening | 23:20 |
| 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:21 |
| mwhudson | maybe something like "see lp.bugs.model.bug.Bug.attachments for where the IIndexedMessage is injected" | 23:22 |
| 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:23 |
| 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:24 |
| 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 | 23:25 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!