| rpadovani | cjwatson, okay, thanks for the explanation :-) | 07:27 |
|---|---|---|
| === danilos` is now known as danilos | ||
| wgrant | cjwatson: When you're around, can we discuss your snap browser branches? | 08:58 |
| cjwatson | wgrant: Sure | 09:57 |
| wgrant | cjwatson: The listing branch has an awful lot of tentacles. | 10:00 |
| wgrant | What was the motivation behind IHasSnaps? | 10:00 |
| wgrant | The IBranchCollection/IGitCollection getSnaps methods are a little neater than the alternative, but I'm not sure IHasSnaps is worth its weight. | 10:01 |
| cjwatson | Hm, so originally it was partly because I was going to use it for SnapAddView | 10:02 |
| cjwatson | But then I ended up attaching those to Branch/GitRef directly | 10:02 |
| cjwatson | And originally I had a bit more per-implementer logic there | 10:03 |
| wgrant | Using it to attach views might be justifiable. | 10:03 |
| wgrant | But those tend to be slightly customised anyway. | 10:03 |
| cjwatson | So it can possibly go away now | 10:03 |
| wgrant | https://code.launchpad.net/~wgrant/launchpad/snap-listings <- here's one I prepared earlier | 10:03 |
| cjwatson | The invention of SnapSet.findByContext was fairly late in the development of that branch | 10:04 |
| wgrant | Ah, that'd do it. | 10:04 |
| wgrant | I also don't really like the IBranchCollection and IGitCollection tentacles, but they're far more contained so if it's not easy to give a method on SnapSet a BranchCollection or GitCollection then they're not unreasonable. | 10:04 |
| wgrant | (I realise this is a standard that code hasn't previously been held to, but I'd prefer the situation not continue to get worse) | 10:06 |
| wgrant | Browser tentacles are somewhat unavoidable, but they're at least not inflating the overhuge classes. | 10:06 |
| cjwatson | The main problem with moving the *Collection bits out is that they need to use _getBranchSelect etc. | 10:06 |
| wgrant | Right. | 10:07 |
| wgrant | It may be okaaaay to make that publicer. | 10:07 |
| wgrant | It's certainly no less gross than putting Snappy bits on the Code collections. | 10:07 |
| wgrant | Then you can use it as a subselect in SnapSet. | 10:07 |
| wgrant | Er | 10:07 |
| wgrant | No more gross. | 10:07 |
| wgrant | It's going to be unfortunately slow either way. | 10:09 |
| wgrant | I guess it may remain quick if the number of snap packages doesn't increase beyond a few thousand. | 10:09 |
| cjwatson | wgrant: Your branch causes lp.snappy.browser.tests.test_snaplisting.TestSnapListing.test_branch_links_to_snaps to fail, because DecoratedBranch isn't a thing that can be passed to SnapSet.findByContext; since there's no longer an interface in use here, lazr.delegates doesn't fix it up for us. What's the least bad way to fix this? Maybe make lp.snappy.browser.hassnaps explicitly check for Decoratedbranch? | 10:25 |
| cjwatson | *DecoratedBranch | 10:25 |
| cjwatson | That's relatively foul, but all the alternatives I can think of are much worse | 10:27 |
| wgrant | cjwatson: Oh, I'd forgotten decoratedbranch was a thing. | 10:29 |
| wgrant | Hmm, uglyu. | 10:29 |
| wgrant | We should really abolish DecoratedBranch at some point, but unwrapping in snappy isn't entirely gross. | 10:29 |
| wgrant | Except that DecoratedBranch exists only in browser. | 10:30 |
| cjwatson | I don't quite get why it isn't just eager loading. | 10:30 |
| cjwatson | And cachedproperties. | 10:30 |
| wgrant | Because cachedproperties in the model weren't a thing then. | 10:30 |
| cjwatson | Oh right, that. | 10:30 |
| wgrant | It's easy enough to abolish now. | 10:30 |
| cjwatson | It's only in browser, indeed, so I can't unwrap it in SnapSet.findByContext. | 10:30 |
| cjwatson | But could possibly unwrap it in lp.snappy.browser.hassnaps. | 10:30 |
| wgrant | Ah yes, indeed. | 10:31 |
| wgrant | That'll do for now. | 10:31 |
| cjwatson | I've pulled your branch and fixed that up. I'll work on the *Collection bits next, and then merge into snap-add-view and make sure everything still works there. | 10:38 |
| wgrant | Thanks. Other than that it looks sensible. | 10:39 |
| wgrant | One day we will be free of ridiculous circular imports. | 10:39 |
| cjwatson | wgrant: Hm, would it be OK to use collection.getBranches().get_plain_result_set()._get_select() in SnapSet? That's all _getBranchSelect does, so it'd save exporting a new interface method | 10:40 |
| cjwatson | Though maybe that also means it's fairly harmless to export. | 10:41 |
| wgrant | cjwatson: I'm not sure you even need _get_select. | 10:41 |
| wgrant | I think you can do .is_in(resultset) | 10:42 |
| wgrant | No leading underscores sounds like an invitation to me. | 10:42 |
| cjwatson | Can you do .is_in(DecoratedResultSet) though? | 10:43 |
| wgrant | I doubt it. | 10:43 |
| wgrant | But get_plain_result_set() is not private. | 10:43 |
| cjwatson | If is_in is given something that's not an Expr, it list()ifies it first. | 10:44 |
| cjwatson | So it might work, but will not be quick. | 10:44 |
| wgrant | Ah, damn. | 10:45 |
| wgrant | _get_select it is, then. | 10:45 |
| wgrant | It even makes it obvious that it's a bit icky. | 10:46 |
| cjwatson | Maybe I should just export _getBranchSelect rather than writing it out again, though. | 10:47 |
| wgrant | Fair. | 10:47 |
| * cjwatson gets more coffee rather than dithering. | 10:47 | |
| cjwatson | Do you have an opinion on the practice of exporting interface methods with leading underscore as an "ok, you don't have to remove the sec proxy, but it's still icky" indicator? | 10:48 |
| cjwatson | I guess we have a few of those ... | 10:48 |
| wgrant | It is evil, but luckily there is sed. | 10:50 |
| wgrant | In fact that method's only referenced in that file. | 10:50 |
| wgrant | Mm | 10:50 |
| wgrant | I guess the ickiness indicator is valuable. | 10:50 |
| wgrant | So I'm not fussed either way. | 10:50 |
| cjwatson | 6 files changed, 19 insertions(+), 111 deletions(-) | 11:11 |
| cjwatson | (disentangling from {Branch,Git}Collection | 11:12 |
| cjwatson | ) | 11:12 |
| wgrant | but how | 11:14 |
| cjwatson | just the disentanglement commit, not the whole thing :) | 11:15 |
| wgrant | yeah, still surprisingly productive. | 11:16 |
| cjwatson | wgrant: All done and pushed, both snap-listings and snap-add-view | 11:26 |
| wgrant | Huh. | 11:31 |
| wgrant | It ended up nearly 500 lines shorter! | 11:31 |
| wgrant | Thanks. | 11:31 |
| cjwatson | Spotting that get{Branch,Repository}Ids were already exported and are nearly the same thing helped. | 11:35 |
| wgrant | ahh. | 11:35 |
| wgrant | Are the three related snap templates identical? | 11:36 |
| cjwatson | Almost. Let me see if I can unify/macro them | 11:37 |
| cjwatson | I wonder if the snap templates should live in lib/lp/snappy/ ? | 11:38 |
| wgrant | That would indeed make sense, if they can be unified. | 11:38 |
| cjwatson | There's precedent for e.g. stuff in code defining pages for registry objects. | 11:39 |
| wgrant | True. | 11:39 |
| wgrant | But better if we can unify them :) | 11:39 |
| cjwatson | Though that might be a bit too odd since *-index are still going to have to link to them. | 11:39 |
| wgrant | Is that odder than the template in Code referencing weird mixins from Snappy? | 11:49 |
| cjwatson | wgrant: Pushed, what do you think? I haven't addressed your review comments yet, that's next | 12:14 |
| wgrant | cjwatson: Looks good. | 12:24 |
| wgrant | Code's running out of Snappy tentacles. | 12:25 |
| cjwatson | All this magic dependency injection, I feel like a Java programmer | 12:26 |
| wgrant | Hm, it doesn't seem to terrible to me. | 12:27 |
| wgrant | too | 12:27 |
| cjwatson | I didn't *actually* say it was terrible :-) | 12:31 |
| cjwatson | wgrant: "There won't always be matching GitRefs, so this preloading won't always find everything." but if they don't exist we don't need to preload them | 12:34 |
| wgrant | cjwatson: But if the preloading is useful, then whatever it is useful to will make queries for the refs that aren't in the cache. | 12:34 |
| cjwatson | Ah, yes. Maybe Snap.git_ref should be a cachedproperty so that I can fill it in here. | 12:36 |
| cjwatson | Actually I suspect that bit of preloading isn't doing anything right now. | 12:37 |
| wgrant | Yep. | 12:39 |
| cjwatson | I'll pull that out and sort it out in a later branch. | 12:42 |
| === timrc_ is now known as timrc | ||
| blr | morning | 20:42 |
| wgrant | Morning blr. | 21:31 |
| blr | hey wgrant | 21:32 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!