/srv/irclogs.ubuntu.com/2009/11/27/#launchpad-reviews.txt

thumpermwhudson: could I talk through https://code.edge.launchpad.net/~thumper/launchpad/edit-commit-msg-link/+merge/15260 with you?01:49
thumpermwhudson: it is very unexciting01:49
thumpermwhudson: the only major change is LPS vs YUI() in many places01:49
thumpermwhudson: I talked this through with mars and he is v.happy that someone else is taking something off his plate01:49
mwhudsonthumper: okay01:56
thumpermwhudson: thanks01:56
mwhudsonthumper: do you want to skype?01:56
mwhudsoni should read the diff first in any case01:56
thumpersure01:56
thumperping me if you want to cover anything01:56
mwhudsonthumper: i'm trying to play with it but things keep getting in my way and then i get distracted02:38
thumper:)02:38
thumperI'll have another soon02:39
thumpernormalizes the review_type in the merge proposal model code02:39
thumperso '  ' -> None02:39
mwhudsonah cool02:40
thumperwaiting for the scanner on that one02:40
mwhudsonthumper: the commit editor doesn't work for me any more in launchpad.dev :(02:58
thumper??02:58
thumperwhat do you mean?02:58
mwhudsonthumper: i click the pencil icon and it works like a link02:59
thumper:)02:59
thumperI know what that is02:59
thumpermake clean build02:59
thumperyui 3.002:59
mwhudsonok02:59
mwhudsonanother thing to delay this review :(03:04
mwhudsonthumper: hm, i was merging it into devel :(03:08
thumper:(03:18
mwhudsonah now it works at last03:25
mwhudsoncomplete with duff looking multi-line editor :-)03:25
thumperhttps://code.launchpad.net/~thumper/launchpad/set-empty-review-type-as-none/+merge/1530203:26
thumpermwhudson: yeah, I think deryck is looking at that one03:27
mwhudsonthumper: in test_pending_review_registrant it's not super obvious to me that the merge_proposal.registrant is who's passed into nominateReviewer03:31
thumperhmm.. yeah03:31
mwhudsonthumper: could you give makeProposalWithReviewer an optional "requester" argument do you think?03:31
thumperand default it?03:32
thumpersure03:32
mwhudsonywah03:32
mwhudsonyes even :)03:32
mwhudsonthanks03:32
mwhudsonthumper: reviewed03:33
thumperta03:34
jtvstub: I thought I could get mwhudson to review my testfix branch, but haven't heard from him in a bit... want to help out?07:12
jtvhttps://code.edge.launchpad.net/~jtv/launchpad/bug-489068/+merge/1530907:12
stubjtv: That shouldn't be an XXX - it should just be a standard comment07:13
jtvFixing...07:13
stubjtv: Wouldn't the real fix be to include boto in the launchpad-developer-dependencies?07:14
jtvstub: well it's not used in the test, and probably shouldn't be07:15
jtvI can just imagine the potential for recursion if this test really fired up an EC2 image.  :)07:16
stubWe try to keep the environments as similar as possible - this is just sweeping this drift between the environments under the carpet where it might bite someone later.07:17
stub(Which is understandable, as it is busted *now* and updating ec2 images seems to take days or longer)07:17
jtvin that case, I can file a bug for having a boto egg (or launchpad-dependency?)07:19
stubYes. I'm wondering if that comment should be an XXX referencing that bug then.07:19
jtvFair enough.07:20
stubapproved07:21
jtvI'll make them separate comments for the two imports.07:21
jtvthanks07:21
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
allenapadeuring: Could you review my multithreaded checkwatches branch please? https://code.edge.launchpad.net/~allenap/launchpad/multithreaded-checkwatches/+merge/1528309:41
adeuringallenap: sure09:41
allenapadeuring: Thanks :)09:41
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
adeuringallenap: the pattern try: return func(...); finally: self._logout() in _interactionDecorator() looks a bit odd. It is not wrong, but jtv spotted a bug in a similar pattern in ec2test. Effectively, the "return" transforms the "Finally" into an "except", so I think it is better to use the latter10:17
allenapadeuring: I don't understand. I thought the thing in ec2test was a try...else?10:18
adeuringallenap: ouch, right. But let me test this "finally" nevertheless...10:19
adeuringallenap: OK, the "finally" code is indeed called! my bad... sorry for the noise10:20
allenapadeuring: Not at all, it's fine :)10:21
adeuringallenap: perhaps I misunderstood something aabout threading, so i may be wrong. but: updateBugTracker() calls self.txn.begin() and later self.txn.commit(). As I understand it, all worker threads share one "self" instance, so don't they begin/commit the same transaction?10:52
allenapadeuring: This was a fun thing for me to work through :) The transactions are stored as thread local vars. I think self.txn is just a reference to the transaction module.10:54
adeuringallenap: that's really interesting. Could you add a comment explaining this so that other people don't ask the same question as I did ;)?10:55
allenapadeuring: Sure, good idea.10:55
adeuringallenap: last question (hopefully...): do you know if calls like self.log.inf() or self.warning() are thread safe?11:09
* adeuring is a bit paranoid regarding threading11:09
allenapadeuring: I don't know actually. I'll check.11:09
allenapadeuring: The logging module has been written to be thread-aware (though I've spotted a race-condition already). I think that's enough.11:13
allenapadeuring: What do you think?11:14
adeuringallenap: yeah, sure. Though it might be worth to file a bug about the race condition11:14
allenapadeuring: Yeah, definitely :)11:14
adeuringallenap: r=me11:21
=== matsubara-afk is now known as matsubara
=== danilo_ is now known as danilos
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
jtvadeuring: got a review for you!  https://code.edge.launchpad.net/~jtv/launchpad/bug-487447/+merge/1531514:23
* adeuring is looking...14:23
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
adeuringjtv: you're adding a permission for public.project. Do you really need that?14:51
=== bigjools-afk is now known as bigjools
=== adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
jtvadeuring: hi, sorry for the delay... yes I want to do that because a project can have a translation group.15:46
adeuringjtv: ah, right, I didn't know that. Anyway, r=me15:47
jtvadeuring: thanks!15:47
adeuringwelcome :)15:48
allenapadeuring: Do you fancy another review today?15:50
adeuringallenap: sure15:50
allenapadeuring: It's a big one at ~1500 lines but there's a lot of removals and refactoring in there. Take a look and see if you feel like doing it. I won't mind at all if you say no :) https://code.edge.launchpad.net/~allenap/launchpad/update-project-group-page-bug-434764/+merge/1532215:51
allenapadeuring: I just don't know how to break it up now :(15:51
adeuringallenap: I'll give it a try ;)15:51
allenapadeuring: Thanks :)15:53
=== beuno is now known as beuno-lunch
noodles775adeuring: time for a one-liner? https://code.edge.launchpad.net/~michael.nelson/launchpad/470181-ppa-inline-style/+merge/1532516:17
adeuringnoodles775: nice distraction from a novel I'm reviewing ;)16:18
noodles775lol16:18
adeuringnoodles775: r=me16:37
noodles775Thanks!16:37
=== beuno-lunch is now known as beuno
adeuringallenap: r=me; 1 nitpick17:21
allenapadeuring: Woohoo! Thank you :)17:22
=== adeuring changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!