| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | Since this is the reviewers channel… if we're going to write the queue in the topic in python, how do I make it follow our coding standards? | 06:11 |
|---|---|---|
| StevenK | Submit a patch | 06:15 |
| === StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge, jtv, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| StevenK | queue.append(StevenK) | 06:15 |
| StevenK | jtv: It can't be a tuple, they're immutable :-) | 06:16 |
| jtv | StevenK: won't queue = queue[1:] still work? | 06:21 |
| StevenK | I think that creates a new instance, doesn't it? | 06:21 |
| jtv | Yes, so what? | 06:27 |
| jtv | We care about the value of queue. | 06:27 |
| StevenK | Yay, semantics :-) | 06:29 |
| * StevenK notes lifeless is apparently OCR today | 06:29 | |
| StevenK | *cough* | 06:29 |
| jtv | Why, dear God, does my neighbor have to be such an enthusiast on the saxophone? | 06:33 |
| StevenK | jtv: Ah, but is he any good? | 06:34 |
| jtv | From the Halls of Montezuma to the Shores of Tripoli… he's a nice guy but he does make me wish he'd make the trip. | 06:34 |
| jtv | It's *just* *not* *a* *saxophone* *melody* | 06:34 |
| jtv | Better than some of the other stuff he's been playing today, I'll grant you, but still. | 06:35 |
| jtv | I'm no expert but there must be better choices of brass for this. | 06:39 |
| lifeless | StevenK: uhh, indeed. OTOH today is insane for me, sorry. | 06:57 |
| lifeless | thats why my name isn't in there. | 06:57 |
| StevenK | lifeless: Mostly teasing, so it's all good | 06:57 |
| lifeless | and it would have been morning only anyway, which finished 6 hours ago | 06:57 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [henninge, jtv, StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | henninge: unexpected breakthrough with my branch; it turned out to be easy after all. Shall I review yours now? | 09:15 |
| henninge | jtv: please do ;) | 09:15 |
| henninge | That's great news! ;) | 09:15 |
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: None || queue = [jtv, StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | henninge: we're talking about your -3 branch, right? I see that -1 is merged and -2 is approved. | 09:19 |
| henninge | jtv: right. -2 is merged, too. | 09:19 |
| * henninge checks that | 09:19 | |
| jtv | OK. | 09:19 |
| jtv | The UI doesn't say it's merged yet. | 09:20 |
| henninge | no, it's not :( | 09:20 |
| henninge | I'll do that now. | 09:20 |
| jtv | (Also, when I ran a Recife branch this morning, I noticed that the statistics said "changed in Ubuntu" on both sides. | 09:21 |
| jtv | ) | 09:21 |
| jtv | BTW we're supposed to open a card per branch when we split things up like this. | 09:21 |
| henninge | right | 09:21 |
| henninge | jtv: of cource, now the diff on the MP is wrong, it contains -2, too. | 09:22 |
| jtv | Maybe we can still set -2 as a prereq on the -3 MP. | 09:22 |
| jtv | henninge: yup, we can. Shall I just do that then? | 09:23 |
| henninge | jtv: please do | 09:23 |
| jtv | ok | 09:23 |
| jtv | Done. | 09:24 |
| henninge | Although I'll land it now anyway, dunno how that affects things | 09:24 |
| jtv | (Through resubmission, so don't be alarmed by any weird emails you may get about it) | 09:24 |
| jtv | Won't matter. | 09:24 |
| henninge | Recife has been update. | 09:26 |
| henninge | d | 09:26 |
| * jtv pulls | 09:27 | |
| jtv | henninge: in _makeTranslation, consider naming "is_other" to be consistent with what we do in the factory. | 09:33 |
| jtv | henninge: oh never mind, I see it has a different meaning. | 09:35 |
| henninge | yes, it makes a "just other" message, not a sharing one. | 09:35 |
| jtv | In a somewhat weird way—why not create a suggestion, then activate it on the appropriate side? | 09:37 |
| henninge | jtv: activate + review. | 09:39 |
| henninge | although, I guess that would work, too. | 09:40 |
| henninge | "markReviewed" and "setFlag" | 09:41 |
| jtv | henninge: well you're doing the latter anyway. But I'm talking about setting just the right flag, instead of setting one, then checking if it's the wrong one, and if so clearing it and setting the other one. That's what I think you do now. | 09:41 |
| henninge | it does | 09:41 |
| henninge | I do | 09:41 |
| henninge | jtv: I can change that. | 09:42 |
| henninge | jtv: how's that: http://paste.ubuntu.com/536216/ | 09:45 |
| henninge | jtv: maybe this reads better ;) http://paste.ubuntu.com/536217/ | 09:47 |
| jtv | henninge: slow down, I was just looking at the last one. :) | 09:47 |
| henninge | tests still pass with it ... | 09:47 |
| henninge | jtv: the second is the same but not a diff, just plain python | 09:48 |
| jtv | Ah. It's fine with me, though an "elif" could probably have been easier. | 09:48 |
| jtv | (though with a bit of repetition) | 09:48 |
| jtv | Never mind. It's fine as you have it. | 09:48 |
| jtv | henninge: next, now that you've betrayed knowledge of what the method does by renaming it, could you provide a docstring for _assertConfirmEmptyPluralOther? I found the test hard to deal with because it assumes familiarity with a pretty large, complicated, underdocumented view class. | 09:50 |
| henninge | Sure, I can do that. | 09:51 |
| jtv | Thanks. | 09:52 |
| jtv | henninge: in test_other_translation, I take it the "other translation" is actually a "current translation on the other side"? | 10:00 |
| henninge | jtv: yes | 10:04 |
| jtv | May be worth saying, to avoid confusion with "another translation" | 10:05 |
| henninge | but "current_translation_on_other_side" is such a long name for a variable. | 10:05 |
| henninge | jtv: I think that if we consistently use "other side" and "other" in our new code, there shouldn't be any confustion. | 10:05 |
| henninge | (eat that "t" if you like) | 10:06 |
| jtv | :) | 10:06 |
| jtv | Fair enough, but it'd be nice to have just one word in there somewhere to indicate that it's other-side, not just other. | 10:07 |
| henninge | I mean "other side" is a concept, just like "imported" was. We did not explain that every time we used it, either. | 10:07 |
| jtv | No need to explain it every time, I agree. | 10:07 |
| henninge | I can add that. | 10:07 |
| jtv | Thanks. | 10:07 |
| jtv | (Meanwhile we seem to be in conflict with db-stable again… I'll see if I can fix that) | 10:07 |
| henninge | jtv: weren't you supposed to be at a dinner soon? | 10:08 |
| jtv | Yes. | 10:08 |
| jtv | So quickly: | 10:08 |
| jtv | Several notes on _set_dismiss_flags. | 10:08 |
| jtv | The docstring says the flags are always set to False. Begpardon? | 10:08 |
| jtv | (Not your fault I guess, but still: begpardon? :) | 10:08 |
| henninge | got your point ;) | 10:09 |
| henninge | "The flags have been initialized to False in the constructor. This method activates the right ones." | 10:10 |
| henninge | jtv: ^ better? | 10:10 |
| jtv | Better! | 10:10 |
| jtv | Also, instead of the nested ifs, maybe just something like this: | 10:11 |
| jtv | has_new_other = other is not None and ( | 10:11 |
| jtv | self.context.date_reviewed is None or | 10:11 |
| jtv | self.context.date_reviewed < other.date_created) | 10:11 |
| jtv | If you think that's easier, that is. If you prefer the ifs, never mind. | 10:11 |
| henninge | usually I don't prefer complicated if constructs. | 10:12 |
| henninge | jtv: much better, thank you. | 10:14 |
| jtv | A hybrid would work as well—keep the outer "if" but remove the inner one. | 10:14 |
| jtv | Oh, nm then, glad you like it. :) | 10:14 |
| * henninge had to read and compare first. ;) | 10:14 | |
| jtv | :-) Another one that could be just slightly nicer to read is: | 10:14 |
| jtv | if len(local_suggestions) == 0 and not has_new_other: | 10:14 |
| jtv | instead of if not(len(local_suggestions) > 0 or has_new_other): | 10:14 |
| jtv | Again matter of taste though. | 10:15 |
| henninge | good idea, just now I stumbled when reading that line. ;) | 10:16 |
| jtv | :) | 10:16 |
| jtv | Finally, I'd leave out the "OK, let's set some flags" comment—since that's what the method does anyway. If we need a comment like that, it's probably a sign that the method gets too long and/or complicated. | 10:16 |
| henninge | ... I had considered removing that, too. | 10:17 |
| henninge | jtv: actually, just figured out another improvement. | 10:18 |
| jtv | ? | 10:18 |
| henninge | has_new_other is not needed, I can assign directly to can_dismiss_other | 10:19 |
| henninge | and just check for len(locals) == 0 | 10:19 |
| jtv | Oh, yes. | 10:20 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: None || queue = [jtv, StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | That way you don't have to keep track of whether your "early escape" condition still applies to the rest of the code in the method. | 10:20 |
| henninge | jtv: in all its glory: http://paste.ubuntu.com/536231/ | 10:20 |
| jtv | glory even? | 10:21 |
| henninge | well, "juice" ? | 10:21 |
| henninge | s/new/newer/ | 10:21 |
| jtv | That really is a much nicer read. | 10:21 |
| * jtv moves to push the button | 10:22 | |
| * henninge gets excited | 10:22 | |
| jtv | done! | 10:23 |
| jtv | thanks for the branch | 10:23 |
| henninge | Yipee! (sp?) | 10:23 |
| henninge | jtv: thanks for the review | 10:23 |
| jtv | I don't think spelling of that word is particularly rigorous. I also just pushed an updated recife with the latest db-stable merged; it had some conflicts outside of translations. | 10:23 |
| henninge | jtv: thank you very much. Enjoy your dinner! | 10:24 |
| jtv | Thanks, bis morgen! | 10:24 |
| henninge | allenap: Nice to see you! | 10:24 |
| henninge | a reviewer that is not having a holiday today ... ;) | 10:24 |
| henninge | allenap: if you start on jtv's branch, you can ask me about it. I hope I can answer ... ;) | 10:25 |
| === matsubara-afk is now known as matsubara | ||
| allenap | henninge: Hi there :) | 11:25 |
| henninge | Hi allenap ;) | 11:26 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue = [StevenK, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| allenap | I'm not very quick off the mark today. | 11:53 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue = [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| StevenK | allenap: Thank you for the review :-) | 12:28 |
| === mrevell is now known as mrevell-lunch | ||
| allenap | StevenK: Welcome :) | 13:29 |
| === mrevell-lunch is now known as mrevell | ||
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | hey allenap! I got an easy one for you: https://code.launchpad.net/~julian-edwards/launchpad/log-parser-bug-680463/+merge/41865 | 14:30 |
| henninge | abentley: Hi! | 14:32 |
| abentley | henninge: Hi! | 14:32 |
| henninge | abentley: Are you able to finish the review for danilo's migration script branch? | 14:33 |
| henninge | https://code.launchpad.net/~danilo/launchpad/migrate-current-flags/+merge/41364 | 14:33 |
| abentley | henninge: Okay. | 14:33 |
| henninge | abentley: I have looked into it and it looks good to me now. I can approve it, too, but did not want to ignore you here. | 14:33 |
| abentley | TBH, I wasn't sure how you guys wanted to handle it. | 14:35 |
| henninge | abentley: danilo's branch is the right one | 14:36 |
| henninge | He is unavailable now, so I am going to land this. | 14:36 |
| henninge | abentley: is that what you meant by "handle it" ? | 14:37 |
| abentley | henninge: No, I mean that when I reviewed, jtv was going to revise it, but I gave him some suggestions on IRC that didn't appear in the review, and I don't know whether they appeared in danilo's branch... | 14:38 |
| henninge | abentley: I would not expect so, no :( | 14:38 |
| henninge | abentley: AFAICT danilo just fixed the problem jtv had indentified. | 14:39 |
| henninge | abentley: that was this channel, right? | 14:40 |
| abentley | henninge: yes. | 14:40 |
| henninge | abentley: I am trying to find the irclogs to that conversation | 14:41 |
| abentley | henninge: I'm just looking at the changes jtv made | 14:42 |
| henninge | abentley: where are you looking? | 14:42 |
| abentley | henninge: http://bazaar.launchpad.net/~jtv/launchpad/migrate-current-flags/ | 14:42 |
| henninge | abentley: that only contains cosmetic changes | 14:44 |
| henninge | over danilo's branch, I mean. | 14:45 |
| abentley | henninge: True. And I will approve it, but I will note that the changes are missing. | 14:45 |
| henninge | abentley: thanks. ;) Do you remember the day and roughly time of day you suggested these changes to jtv? | 14:47 |
| abentley | henninge: LP says it was 2010-11-22 and it was probably 13:00 UTC or so. | 14:47 |
| allenap | bigjools: I'll look at that one now! | 14:49 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | allenap: it's a little gross but I have no better ideas | 14:49 |
| allenap | bigjools: It's cool. Why do you need the & 0xffffffffL line? If it's a number from a 32-bit field then that seems superfluous? | 15:02 |
| bigjools | allenap: I think it's to cope with sign. I shamelessly copied it from the gzip module source | 15:02 |
| bigjools | without thinking much about it, admittedly | 15:03 |
| allenap | bigjools: Oh yes, weird. +1 | 15:04 |
| bigjools | allenap: what do you think? | 15:05 |
| bigjools | it'll blow up if we read >~4.2GiB anyway | 15:05 |
| allenap | bigjools: I think it can stay. I can't see that it makes a difference, but my bit manipulation is rusty so I might be missing something. | 15:09 |
| bigjools | allenap: mine also :) | 15:10 |
| bigjools | I figured there was probably a good reason for it to be in the original code | 15:10 |
| === salgado is now known as salgado-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara-lunch is now known as matsubara | ||
| === allenap 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 | ||
| === salgado-lunch is now known as salgado | ||
| salgado | anybody interested in reviewing a slightly long diff which exposes blueprints on the API? | 19:48 |
| mwhudson | salgado: maybe, but i can't promise a response time better than "today" | 19:55 |
| salgado | mwhudson, that'd be great; I wasn't expecting anybody would be able to review it before my EOD anyway. :) | 19:57 |
| === salgado is now known as salgado-afk | ||
| mwhudson | salgado-afk: sorry about that | 20:31 |
| salgado-afk | mwhudson, about what? | 20:46 |
| mwhudson | disappearing on you | 20:46 |
| salgado-afk | mwhudson, oh, no worries. did you get my reply? | 20:47 |
| mwhudson | salgado-afk: only | 20:48 |
| mwhudson | <salgado> mwhudson, that'd be great; I wasn't expecting anybody would be able to review it before my EOD anyway. :) | 20:48 |
| mwhudson | did you request a review? | 20:48 |
| salgado-afk | mwhudson, oh, good point, I forgot to tell you where my branch is. do you want me to ask a review from you? | 20:49 |
| mwhudson | i think i found it | 20:49 |
| mwhudson | https://code.launchpad.net/~salgado/launchpad/expose-blueprints/+merge/41898 | 20:49 |
| salgado-afk | yeah, that's the one. :) | 20:50 |
| mwhudson | cool | 20:50 |
| * mwhudson claims | 20:50 | |
| === matsubara is now known as matsubara-afk | ||
| wallyworld | thumper: sorted(builds, key=attrgetter('id')) won't work - the named tuple doesn't have an id attribute :-) | 23:12 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!