| === vednis is now known as mars | ||
| wallyworld_ | StevenK: are you able to take another look at those recipe build listing branches? - i've made some changes as suggested by you and tim | 04:39 |
|---|---|---|
| jtv | Hi gmb, got a very light review for you, with a bigger one to follow: https://code.launchpad.net/~jtv/launchpad/recife-pre-stats/+merge/42079 | 07:42 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | No reviewers today? | 08:00 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | stub: are you as busy as henning is? Need some reviews! | 08:23 |
| henninge | jtv: what makes you think I am busy? ;-) | 08:24 |
| henninge | No staging - no QA | 08:24 |
| jtv | henninge: okay, then I need some reviews! | 08:26 |
| henninge | let's do one at a time ... ;) | 08:26 |
| === henninge changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [jtv, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | Yes, let's. :) | 08:26 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [ jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | wow, in-line diffs for updates. | 08:29 |
| jtv | Yes, we have those now. | 08:32 |
| henninge | jtv: very nice simplification of the test. | 08:43 |
| henninge | jtv: I just don't see what you describe as "runs in both old-model and new-model statistics calculation." | 08:44 |
| jtv | In all honesty, I did it to get the test to start up faster. :-) | 08:44 |
| jtv | I clear both flags as an alternative to deleting the message. | 08:44 |
| henninge | ah! so lines 148-151 could use a comment I think ;) | 08:46 |
| henninge | jtv: but apart from that, r=me ;) | 08:46 |
| * jtv looks | 08:46 | |
| jtv | OK, I'll note that. | 08:46 |
| henninge | thanks | 08:47 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [ ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | ok, the next branch is a bit larger | 08:48 |
| jtv | Yup, though this pre-branch got it under the 800 line. | 08:48 |
| jtv | henninge: actually, instead of a comment, how about I just fix getPOFilesWithTranslationCredits? | 08:49 |
| henninge | in what way? | 08:50 |
| jtv | By making it look at the translation side that's appropriate for the POFile, rather than always at is_current_ubuntu. | 08:52 |
| jtv | (In other words, updating it to the Recife model). | 08:52 |
| jtv | By the way, my router had come back to life but went back into a coma again, so I'm on a slow edge connection today. Makes it hard to get to the kanban board. :( | 08:52 |
| henninge | jtv: so I guess voice chat is out of the question? | 08:55 |
| jtv | henninge: pretty much :( | 08:55 |
| henninge | :( | 08:56 |
| henninge | jtv: if the test works as it is now you should finish the statistics stuff first - although I fear that is all inter-related. | 08:59 |
| jtv | henninge: might as well finish this while you're reviewing—it's not a big change. | 09:00 |
| jtv | Better this than forget to fix it! | 09:00 |
| henninge | that's cool, too | 09:01 |
| jelmer | g'morning! | 09:03 |
| henninge | Hi jelmer ;) | 09:03 |
| jelmer | henninge: Can I add another branch to the queue? | 09:03 |
| henninge | jelmer: you always can ydd a branch to the queue ... :-P | 09:04 |
| henninge | s/ydd/add/ | 09:04 |
| === jelmer changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [jelmer ] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jelmer | henninge: thanks (-: | 09:05 |
| henninge | jtv: what does that change "Merging preparatory branch" mean? Where do all the property changes come from? | 09:05 |
| jtv | henninge: I merged the branch you just reviewed, but since that one was branched off db-devel whereas this branch was branched off recife (which in turn was branched off an even older db-stable), it brought in a lot of other changes that won't show up in a diff against db-devel. | 09:06 |
| henninge | I see | 09:07 |
| jtv | In other words: irrelevant updates. :-) | 09:07 |
| === gmb` is now known as gmb | ||
| === jtv changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [jelmer, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | jtv: what's the trick in this? | 10:24 |
| henninge | 137+ for count, in IStore(self).execute(query): | 10:24 |
| henninge | 138+ return count | 10:24 |
| henninge | Would [0] not do the same? | 10:24 |
| henninge | or I think there is even a "first" method. | 10:24 |
| jtv | Only if I listify first. I wish I had a really concise way of stating it, but I'm willing to go with any suggestion you may have. :) | 10:25 |
| jtv | Oh, first, that might work. | 10:25 |
| henninge | jtv: and what about "one"? | 10:26 |
| henninge | I am still trying to figure out why you'd only want the first, or why the query returns more than one. | 10:26 |
| jtv | There is only one. | 10:27 |
| jtv | henninge: first() does the trick. Also magically turns the tuple-shaped row into a single int, which is what I really want. | 10:28 |
| henninge | jtv: one() does not do that? | 10:28 |
| jtv | The whole trick to that code was that I had a result set that looked like an iterator over [(n)], and I just wanted the n. | 10:29 |
| jtv | Oh, hang on, first() did _not_ work. What I saw was a screenful of errored tests. | 10:30 |
| jtv | There is no "one" (or first) on this type of result set. | 10:31 |
| jtv | Duck-typing to the descue. | 10:31 |
| jtv | Or whatever the opposite of rescue is. | 10:31 |
| henninge | jtv: what is "this type of result set" ? | 10:31 |
| jtv | PostgresResultSet. | 10:32 |
| jtv | Store.execute returns something different than Store.find. | 10:32 |
| henninge | jtv: it has get_one, though | 10:33 |
| henninge | http://people.canonical.com/~jkakar/storm/storm.databases.postgres.PostgresResult.html | 10:33 |
| jtv | ah! | 10:34 |
| jtv | Now trying return IStore(self).execute(query).get_one()[0] | 10:35 |
| jtv | That works. | 10:35 |
| jtv | Thanks. I always look through the documentation in the source code, but didn't find anything helpful. | 10:35 |
| jtv | Change pushed. | 10:41 |
| henninge | cool | 10:47 |
| jtv | Meanwhile, could anyone else review this other very small branch? https://code.launchpad.net/~jtv/launchpad/recife-getpofileswithtranslationcredits/+merge/42083 | 10:53 |
| jtv | jelmer, is that something you could do? | 10:54 |
| jelmer | jtv: Sure, should we trade branches? Mine is very small as well. | 10:55 |
| jtv | jelmer: sure! | 10:55 |
| jtv | Got a direct link? My connection's so slow right now it'd save me a lot of time. | 10:56 |
| jelmer | https://code.launchpad.net/~jelmer/launchpad/681974-building-commit/+merge/42081 | 10:56 |
| * jtv goes into a frenzy of browsing attempts | 10:57 | |
| === matsubara-afk is now known as matsubara | ||
| jtv | jelmer: good cover letter. I'm surprised it's the buildd manager confusing the upload processor by moving the file before committing, and not another way around. | 11:02 |
| jelmer | jtv: This is happening with binary builds that the buildd manager has fetched from the builders. | 11:03 |
| jelmer | jtv: There is another archive uploader instance that processes the source uploads from users, which the buildd manager then sends off to the builders. | 11:04 |
| jtv | jelmer: ah so the data flows in the opposite direction from what I thought. | 11:08 |
| jtv | Depending on what the upload processor's transactions look like and even which isolation level it uses, there may _conceivably_ still be race conditions but even then this should narrow it own. And I'm not assuming that you got it wrong anyway. :) | 11:09 |
| jtv | I'm also assuming that you're not committing in some untenable state, and therefore r=jtv. | 11:10 |
| jelmer | jtv: Thanks :-) | 11:12 |
| jelmer | jtv: I don't understand why line 27 of your change is now necessary, how is it related to your other changes? | 11:17 |
| jelmer | + clauses.append(POTemplate.id == POFile.potemplateID) | 11:17 |
| jtv | jelmer: yes it is a bit obscure. It pulls POTemplate into the join. | 11:19 |
| jtv | I did reference it, but in a nested query. And so that join doesn't get pulled into the outer query. | 11:19 |
| jelmer | ahh, I see | 11:19 |
| jtv | The ORM probably couldn't do that if it wanted, because it'd be ambiguous whether I wanted it in the outer query or just the inner one. | 11:20 |
| jtv | (In this case it doesn't matter much, but I'm assuming in a hand-wavy way that it might in other cases) | 11:21 |
| jelmer | jtv: yeah, I can see how that could make a difference. | 11:21 |
| jelmer | jtv: That was all I had. r=me | 11:22 |
| jtv | dankuwel | 11:22 |
| jtv | phew, my router is working again | 11:51 |
| === jtv changed the topic of #launchpad-reviews to: n call: henninge || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jtv changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | jelmer: if you use "ec2 land," there's no need to include the [r=…] tag etc. in your commit message. That gets added automatically. | 12:11 |
| jelmer | jtv: I didn't add anything, ec2 land did! | 12:12 |
| jtv | oh! | 12:12 |
| jelmer | I guess it's now modifying the commit message on lp before send it off to ec2. | 12:12 |
| jtv | there's no stopping progress, I suppose | 12:12 |
| jelmer | :) | 12:12 |
| jtv | That could actually be helpful when landing fails because of testfix. | 12:13 |
| jtv | No endless re-composing your commit message for every landing attempt. | 12:13 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | jelmer, jtv: thanks guys for taking each other's reviews. | 12:34 |
| henninge | jtv: r=me with a few comments to consider. ;-) | 12:34 |
| * jtv jelmer & henninge all bow at each other | 12:34 | |
| jtv | henninge: great, thanks—sorry for the heavy review loa | 12:34 |
| jtv | d | 12:34 |
| henninge | :-D | 12:34 |
| henninge | jtv: np. I will see that I get the migration script run now that staging is alive again. Or did you already take action? | 12:35 |
| jtv | henninge: no, I didn't know it was up! | 12:35 |
| jtv | henninge: you're right that there's no point to renaming that method I'm replacing—I just did it to keep your diff to a reasonable size, and left cleaning up the now-unused method as one of the TODO items. | 12:43 |
| jtv | I can remove it now if you're willing to rubber-stamp it—there are no calls to it. | 12:43 |
| henninge | jtv: that's what I meant - no calls. ;-) rs=me | 12:44 |
| jtv | Thanks. | 12:46 |
| === mrevell is now known as mrevell-lunch | ||
| === NCommand1r is now known as NCommander | ||
| === matsubara is now known as matsubara-lunch | ||
| abentley | henninge: Could you please review https://code.launchpad.net/~abentley/launchpad/bzr-2.2.2/+merge/41981? | 14:05 |
| henninge | abentley: r=me | 14:13 |
| === mrevell-lunch is now known as mrevell | ||
| === salgado is now known as salgado-lunch | ||
| === bac` is now known as bac | ||
| === matsubara-lunch is now known as matsubara | ||
| === salgado-lunch is now known as salgado | ||
| jam1 | henninge: can I get a quick review of: https://code.edge.launchpad.net/~jameinel/launchpad/lp-service/+merge/42129 | 15:46 |
| jam1 | That was already merged and approved into db-devel, but I had meant to target devel | 15:46 |
| === jam1 is now known as jam | ||
| jam | hmm... the diff appears to be empty, maybe db-devel has already become devel? | 15:47 |
| henninge | jam: the branch's status is "Merged" | 15:48 |
| jam | anyone know when that happened? l-osas said the functionality wasn't present last Wed | 15:48 |
| jam | henninge: right, it *was* merged into db-devel, but I meant to target 'devel' | 15:49 |
| henninge | jam: when did that happen? Merging into db-devel, I mean? | 15:49 |
| jam | I thought I saw that db-devel was going to be merged/pushed over devel, but I don't know for sure that happened | 15:49 |
| jam | henninge: *My* branch was merged into db-devel probably 2 weeks ago, or so | 15:49 |
| henninge | jam: it happened on the 16th | 15:49 |
| jam | I didn't realize I had the wrong target unti the 24th | 15:49 |
| jam | (well, I didn't realize until today :) | 15:50 |
| henninge | jam: devel and db-devel are merged before each roll-out. | 15:50 |
| jam | I was trying to get someone to use the new code, and they said it didn't work on the 24th. | 15:50 |
| jam | but right now, it appears to be properly merged into devel | 15:50 |
| jam | which is confusing me | 15:50 |
| henninge | jam: it was merged on the 9th as r9951, r9973 was rolled out (if I remember correctly) | 15:53 |
| henninge | jam: so, yes, your code is deployed. | 15:53 |
| jam | henninge: thanks. Now I have to figure out why it wasn't available on qastaging... | 15:54 |
| jam | or if something else weird is going on | 15:54 |
| jam | anyway, thanks for your help | 15:54 |
| henninge | jam: it is there for sure (unless it was reverted by a leter revision) | 15:54 |
| henninge | s/leter/later/ | 15:54 |
| jam | henninge: I just cleared out the mp, sorry about the noise | 15:55 |
| salgado | hi henninge. can you review a trivial one for me? | 16:28 |
| === henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | salgado: sure | 16:29 |
| henninge | last onen for today ;) | 16:29 |
| salgado | henninge, https://code.launchpad.net/~salgado/launchpad/refactor-blueprints-tests/+merge/42139 | 16:29 |
| salgado | thanks! | 16:29 |
| bac | hi jcsackett, sorry for taking so long but i think your branch looks great | 16:52 |
| jcsackett | bac: cool! glad to hear it. | 16:52 |
| henninge | salgado: r=me ;) | 16:59 |
| salgado | thanks henninge | 17:00 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| salgado | abentley, how about a trivial one to start the day? ;) | 17:17 |
| salgado | https://code.launchpad.net/~salgado/launchpad/expose-blueprint-dependencies/+merge/42145 | 17:17 |
| === benji is now known as benji-lunch | ||
| abentley | salgado: sorry, was lunching. | 18:05 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: salgado || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| salgado | abentley, np. I've just pushed another tiny change there, though, so you might want to wait for the diff to update | 18:06 |
| abentley | salgado: Is this test refactoring something I should review? | 18:09 |
| salgado | abentley, the ones to use assertContentEqual? Yes, although they're not related to the changes on the branch I thought you wouldn't mind reviewing them together? | 18:10 |
| === benji-lunch is now known as benji | ||
| abentley | salgado: Sure. I just noted they were merged from another branch, so i wasn't sure. | 18:11 |
| salgado | but the "merge test refactoring from other branch" has landed on devel already | 18:11 |
| salgado | abentley, I just had to merge that directly from the branch because at that time it wasn't on devel | 18:12 |
| abentley | salgado: So you could have set that other branch as a prerequisite branch. | 18:13 |
| salgado | abentley, yeah, although it wasn't really. I just merged to make sure there'd be no conflicts | 18:14 |
| abentley | salgado: But now that you've merged it, you can't land this branch until that one's approved. | 18:14 |
| salgado | abentley, but the other one has landed already | 18:15 |
| abentley | salgado: now, it has, and I see that in the updated diff. I was looking at an older one a minute ago. | 18:15 |
| salgado | it landed 15 mins ago or so. but 1h ago it hadn't landed and I merged to make sure there'd be no conflicts | 18:16 |
| salgado | abentley, sorry for the confusion | 18:16 |
| abentley | salgado: do you know about lp.testing.ws_object? It's a more general version of getSpecOnWebservice. | 18:21 |
| salgado | abentley, nope, I didn't know about that | 18:21 |
| * salgado checks | 18:21 | |
| abentley | salgado: The functionality looks fine. At 23, you should use a multi-line import. r=me. | 18:23 |
| salgado | abentley, I could use that in getSpecOnWebservice() but I don't see much benefit in doing so | 18:25 |
| salgado | abentley, oh, so multi-line imports are to be used even when they're not necessary? | 18:25 |
| abentley | salgado: yes. | 18:25 |
| abentley | salgado: I don't think you need getSpecOnWebservice at all. | 18:25 |
| salgado | you mean to inline its functionality everywhere? | 18:26 |
| abentley | salgado: I mean use ws_object instead. | 18:26 |
| salgado | that's basically the same as I still need to get the Launchpad instance. although I could store that as an instance variable on the testcase class | 18:27 |
| salgado | that doesn't sound like a good idea | 18:27 |
| salgado | abentley, or do you mean something else? | 18:28 |
| abentley | salgado: It just seems like wasted effort to have different functions for getting objects via the web service, one for each type of object, when you could just have one. | 18:29 |
| abentley | salgado: If you'd like to enhance ws_object so that the launchpad parameter is optional, that would be a nice improvement for everyone. | 18:32 |
| salgado | abentley, I'm not sure that'd help in this case as these tests need a 'devel' Launchpadlib and I don't think that should be the default | 18:33 |
| lifeless | you could curry something | 18:33 |
| salgado | curry? | 18:34 |
| abentley | salgado: so we could have a parameter on ws_object to specify the launchpadlib version. That would also be helpful for everyone. | 18:37 |
| abentley | salgado: I don't think wanting to vary the launchpadlib version is a good reason to have different implementations of ws_object for different objects. | 18:38 |
| abentley | salgado: At most, it's a reason to have different versions of ws_object for different launchpadlib versions. | 18:39 |
| salgado | I don't think so either; I'm just saying that the current one is not a good fit | 18:39 |
| salgado | but I can change it as you suggest | 18:39 |
| abentley | salgado: I've already approved this, so it's only a suggestion to consider. I just think it's better to spend effort in ways that help the whole team, where we can. | 18:40 |
| salgado | agreed, and that's why I'm happy to do the change you suggested. :) | 18:41 |
| abentley | salgado: cool. | 18:42 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-afk | ||
| salgado | abentley, the annoying thing is that the launchpad object is the first argument to ws_object() and it doesn't make sense to make both arguments optional so I'd have to change the method's signature and update all the callsites. :( | 19:03 |
| abentley | salgado: It's true. | 19:04 |
| salgado | abentley, I think I'll save this work for later, then | 19:05 |
| abentley | salgado: sure. | 19:05 |
| jcsackett | abentley: have one to throw on your queue. https://code.edge.launchpad.net/~jcsackett/launchpad/distribution-enum-selection-677536/+merge/42161 | 20:06 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | jcsackett: ack | 20:06 |
| === salgado is now known as salgado-afk | ||
| === Guest66935 is now known as EdwinGrubbs | ||
| abentley | jcsackett: r=me | 21:31 |
| jcsackett | abentley: thanks! | 21:31 |
| === abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gary_poster | abentley: 4 lines when you get a chance: https://code.launchpad.net/~gary/launchpad/ec2lucid/+merge/42179 | 21:38 |
| abentley | gary_poster: r=me | 21:39 |
| gary_poster | thanks abentley | 21:39 |
| === jcsackett_ is now known as jcsackett | ||
| === abentley changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!