| thomi | wgrant: I fixed all your concerns in that branch - care to re-review? | 01:13 |
|---|---|---|
| blr | thomi: more a case of cornice not having any idea how to deal with that case for a resource | 01:15 |
| blr | probably need to look at the source | 01:16 |
| wgrant | blr: Could you use cornice with object traversal rather than URL dispatch? | 01:16 |
| thomi | blr: ahhh, I've never used cornice, so I may not be much help | 01:17 |
| blr | wgrant: hmm possibly | 01:17 |
| wgrant | You could have a traverser that consumes segments until it hits something that matches a ref or cannot possibly lead to a ref. | 01:17 |
| wgrant | We do that in LP in several places. | 01:18 |
| blr | sounds like what we need here, will keep digging. | 01:18 |
| wgrant | And for variable-length path pieces you have little other choice. | 01:18 |
| wgrant | They occur rarely in most types of applications, of course. | 01:18 |
| blr | right, this isn't something normal people should need :P | 01:19 |
| wgrant | thomi: Looks good, thanks. Can you revise the commit message to describe more accurately that you're splitting it out of +editemails? | 01:20 |
| thomi | yup | 01:20 |
| thomi | wgrant: how's that? | 01:21 |
| wgrant | thomi: It's in buildbot. Thanks. | 01:24 |
| thomi | cool - my first visible fix to lp :D | 01:25 |
| wgrant | It will be good to be able to change mailing list subs without having to reauthenticate. | 01:29 |
| * wgrant lunches. | 01:30 | |
| thomi | wgrant: crap - I just realised I forgot to link the bugs. Is it too late to do that now? | 01:34 |
| wgrant | thomi: yes, but i linked the two you filed earlier :) | 02:10 |
| thomi | wgrant: ugh, thanks | 02:11 |
| thomi | wgrant: just want to make sure that it's OK to use purge-inbox.py from lp-dev-utils on the staging inbox? (I'll keep 1 weeks worth) | 03:02 |
| thomi | I'm taking your silence as a tacit agreement :D | 03:05 |
| wgrant | thomi: Yep, I normally prune to two or three days. | 03:16 |
| wgrant | If someone expects mail to stay in the staging mailbox, that's their own problem :) | 03:16 |
| thomi | yeah - even a week's worth is 34k mails :( | 03:16 |
| wgrant | Hm, shouldn't normally be that much. Odd. | 03:16 |
| thomi | hmm, no, something's not right | 03:18 |
| thomi | I'm purging mails, but offlineimap is still downloading them | 03:18 |
| wgrant | purge-inbox.py probably doesn't expunge until the end. | 03:19 |
| wgrant | offlineimap likely downloads anything that hasn't been expunged. | 03:19 |
| thomi | I'm waiting till purge-inbox has finished | 03:19 |
| cjwatson | wgrant: git-lookup, path/segments> this was because it's convenient that way for the callers; the ones that pass an iterable of segments do so because they're expecting to have more to traverse afterwards. (For example my browser Person traversal code uses this.) Also with a path it's inconvenient to indicate what's left, whereas with an iterable of segments we can just consume the used ones. | 09:53 |
| wgrant | cjwatson: Right, sounds fine. | 09:56 |
| wgrant | Right, just need to sort out the cache maintenance garbo jobs on Monday, and translations should all render fast, except for ddtp-ubuntu which should render in about two seconds. | 10:38 |
| cjwatson | wgrant: I'm having some difficulty working out what you mean in your git-finish-sharing review. The method you're commenting on doesn't handle specs, and gitrepositories_by_id is needed to compute visible_gitrepository_ids. What are you trying to solve there? | 10:56 |
| cjwatson | wgrant: Do you mean something like http://paste.ubuntu.com/10446516/, and relying on the Storm cache to make it efficient that we're walking over the objects twice as often? | 10:56 |
| cjwatson | wgrant: And likewise for getVisibleArtifacts? | 10:56 |
| wgrant | mrh | 10:59 |
| wgrant | does that really not do specs? | 10:59 |
| wgrant | sigh | 10:59 |
| cjwatson | Apparently not | 10:59 |
| wgrant | Though it isn't used except for branches, IIRC. | 11:00 |
| wgrant | Indeed. | 11:00 |
| cjwatson | Yeah, and git won't need it even with subscriptions because we have no stacking. | 11:01 |
| wgrant | s/specs/branches/ in my comment, the rest stands. | 11:01 |
| wgrant | Yeah. | 11:01 |
| cjwatson | But anyway, presumably the same kind of thing applies to getVisibleArtifacts. | 11:01 |
| wgrant | The only "problem" I was trying to solve was that it was hideous and convoluted. | 11:01 |
| wgrant | We construct a map because reasons. | 11:02 |
| cjwatson | It seems more efficient to construct a map, but maybe that's a meaningless microoptimisation. | 11:02 |
| wgrant | uHeh | 11:03 |
| wgrant | At the scale this method is used at, the overhead of constructing the dict is probably greater than the penalty of avoiding it. | 11:03 |
| cjwatson | OK, I'll see how it all looks without. | 11:04 |
| wgrant | (though the old implementation walks through the original list twice anyway, due to the two keys() calls. so it's really very little difference) | 11:04 |
| wgrant | The only reasonable optimisation I might consider is making visible_foo_ids a set. | 11:05 |
| cjwatson | That's not going to actually try to look up attributes of database objects. But hopefully the Storm cache will be enough. | 11:05 |
| wgrant | But I'd not do that in this case because the benefit is minimal and it would cause a linebreak :) | 11:06 |
| wgrant | Oh, well, if the input parameters are Storm resultsets then it could duplicate work, that's true. | 11:06 |
| wgrant | But the objects won't be invalidated unless they somehow get evicted from the cache, and even then accessing the PK won't cause a re-query. | 11:07 |
| wgrant | Anyway, I'm not fussed either way. My suggestion was just a lot more readable, IMO. | 11:07 |
| cjwatson | Looks like they're always real lists, and in the getVisibleArtifacts case they're always one-element lists. | 11:08 |
| cjwatson | At least in model code. | 11:08 |
| cjwatson | (And browser.) | 11:08 |
| cjwatson | I have some time while buildbot churns :-) | 11:08 |
| cjwatson | getVisibleArtifacts doesn't even use the values except for bugs, meh. | 11:11 |
| cjwatson | OK, simplification pushed now. | 11:28 |
| cjwatson | srsly buildbot | 14:26 |
| cjwatson | wgrant: I had a stab at your other two code branches, but I don't think I'm remotely qualified to review https://code.launchpad.net/~wgrant/launchpad/bug-736005-trivialise/+merge/250447 | 17:32 |
| cjwatson | OK, so I think I have the turnip charm deployable locally again and actually running the API service. I'll work on porting my little lp.code.githosting module over to the new code on Monday, and then it'll be possible to finish off xmlrpc tests and push that next branch. | 18:23 |
| wgrant | cjwatson: Thanks. The person most qualified to review that is either a statistician or voodoo priest. | 21:45 |
| cjwatson | stub: https://code.launchpad.net/~cjwatson/launchpad/upgrade-requests/+merge/251337 just in case there was some reason you didn't complete that upgrade | 23:04 |
| cjwatson | but with that I just managed to push a new repository to a juju deployment of turnip, so back in business there \o/ | 23:07 |
| cjwatson | just one more small patch to push to the charm for that when I get a moment, and then back to the remaining 2000-ish lines of git-megabranch | 23:08 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!