/srv/irclogs.ubuntu.com/2010/06/29/#launchpad-reviews.txt

=== jtv1 is now known as jtv
=== matsubara-afk is now known as matsubara
bachi abentley, can you look at https://code.edge.launchpad.net/~bac/launchpad/bug-597809/+merge/28690 ?13:19
abentleybac, sorry, forgot to update the topic.13:20
=== bac changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== bac changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bacabentley: ah, right, you were yesterday.  sorry.13:20
=== 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
=== bac changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bacleonardr or mars:  are either of you reviewing today?13:26
leonardrbac: good question. i guess i'm doing it today--i remember being sick last tuesday13:27
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardrbac, give me the link to your branch13:27
bacthanks leonardr.  i have a trivial branch to start your day.13:27
bachttps://code.edge.launchpad.net/~bac/launchpad/bug-597809/+merge/2869013:28
leonardrbac: why should the privacy sprite not appear on private team pages? is there some other code now that takes care of that?13:31
bacleonardr: the privacy sprite is a padlock.  it was never intended to decorate the "This team is private" message.  instead we have two other decorations.  one is a hashed rule that goes down the left side of the page.  the other is the use of the same hash across the top of the privacy portlet with the text "This team is private".  it is consistent with the presentation for bugs and branches.13:33
bacleonardr: the <div> i removed that rendered the padlock was leftover from the pre-3.0 design13:33
leonardrok, r=me13:33
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bacthanks13:33
rinzeleonardr: Hi, can I add another branch to the queue?14:38
leonardrrinze, sure14:38
leonardrgive me the link14:38
rinzehttps://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/2875014:39
rinzeleonardr: Sorry, I just noticed I proposed against lp:launchpad by accident14:39
rinzeleonardr, I'll get back to you14:40
leonardrsure14:41
rinzeleonardr: https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/2875114:42
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardrrinze, i must ask some basic questions15:00
leonardrwhat makes an architecture "restricted"15:00
leonardr?15:01
rinzeleonardr: Some architectures (such as ARM) only have a limited number of slow builders so we don't want to make them available to everybody who is building openoffice in their PPA15:01
leonardrok15:02
rinzeleonardr: Those architectures are marked as restricted in the database and archives that need to build on those architectures need an explicit entry in the archivearch database table.15:02
leonardrrinze:15:05
leonardr+ terms.append(SimpleTerm(family, token=family.name,15:05
leonardr45+ title=family.title))15:05
leonardri think our coding style requires putting all the arguments to SimpleTerm on a single new line15:06
rinzeah, ok15:06
jtvleonardr, I have an oversized branch...  think it's something you could review?  https://code.launchpad.net/~jtv/launchpad/recife-limited-setcurrenttranslation/+merge/2871515:09
=== henninge_ is now known as henninge
jtvhenninge: maybe you could review the branch we discussed?  It's almost twice as big as it's supposed to be, I'm afraid; those tests are pretty exhaustive.15:37
henningejtv, leonardr: I am taking it.15:38
jtvthanks henninge15:39
leonardrrinze: what happens if you get the enabled_restricted_families_collection_link. are IProcessorFamily objects actually published in the web service?15:51
rinzeleonardr: That's a good question, I hadn't actually considered that - just changed the arm_builds_enabled variable which was exposed.15:52
rinzeleonardr, it looks like it doesn't actually work because of IProcessorFamily not being available so I should probably remove the exported() bit.15:53
leonardrrinze: ok, either way you are breaking the backwards compatibility of the web service.15:54
leonardrin the unlikely event that someone wrote a web service client that checks .arm_builds_enabled15:54
leonardrwe have two options15:54
leonardryou can reimplement arm_builds_enabled as a property, export in 'beta', and de-export it in 'devel'15:55
leonardr(i can show you how to do this)15:55
leonardror, we can decide that it's so unimportant that we can simply remove it retroactively with no worries15:56
leonardrflacoste, your opinion? -^15:56
rinzeleonardr: Sorry, I'm being very clumsy today15:56
leonardrrinze: np, it's a complex change15:56
rinzeIt looks like arm_builds_enabled wasn't actually exported, but I made a mistake in making enabled_restricted_families exported.15:56
leonardraha15:57
leonardrok, so simply don't export it, and there will be no backwards compatibility issues15:57
leonardryou may want to export it later, but no need to do it now15:57
leonardrflacoste, never mind15:57
flacosteleonardr: ack15:57
rinzeleonardr: right - and thanks for pointing that one out15:57
leonardrrinze: r=me with changes. i'll summarize in the review15:59
rinzeleonardr: thanks!16:00
leonardrrinze, review written16:04
leonardrbe sure when you remove exported() to also remove the extra links from your tests16:04
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
rinzeleonardr: will do16:08
rinzeleonardr: could I add another (simpler) branch to the queue?16:22
leonardrrinze, sure16:23
rinzeleonardr: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/394798-auto-buildd-secret/+merge/2875916:23
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-lunch
=== sinzui changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
rockstarleonardr, may I hop on your queue?17:17
leonardrrockstar, sure add it17:17
=== rockstar changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: rinze || queue: [sinzui, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardrrockstar, give me the mp url as well17:19
rockstarleonardr, putting together the mp right now.17:25
leonardrgreat17:25
leonardrrinze, what happens if you specify a build secret for a public archive? is that tested in code that you didn't touch?17:26
rinzeleonardr: yeah, that's already disallowed by code I didn't touch (though I'm not sure if it's tested)17:27
leonardrok17:28
leonardrr=me17:28
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: sinzui || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardrsinzui, what's the branch you want reviewed? registry-tales-2?17:29
rinzeleonardr, thanks17:30
sinzuiyes please17:30
leonardrsinzui, r=me17:42
sinzuithanks17:42
=== matsubara-lunch is now known as matsubara
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
rockstarleonardr, https://code.edge.launchpad.net/~rockstar/launchpad/delete-recipes/+merge/2877317:56
leonardrrockstar: i was just about to ask18:05
leonardrrockstar, "if self.buildqueue_record:" should be "if self.buildqueue_record is not None:"18:15
rockstarleonardr, ah, yeah, that's probably better.18:15
leonardrrockstar: do test_destroySelf sets up a recipe build that has no associated build queue record, to test your new conditional?18:16
rockstarleonardr, yeah, it does.18:17
rockstarleonardr, I actually wrote the test to reproduce the bug first, and then fixed the code to make the test pass properly.18:17
leonardrok, r=me with that minor change18:17
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== salgado is now known as salgado-lunch
rockstarleonardr, thanks.18:20
=== salgado-lunch is now known as salgado
EdwinGrubbsleonardr: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-576388-proposing-invited-team/+merge/2878619:35
leonardredwingrubbs: sure19:54
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bdmurrayleonardr: I could use a review of https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/2854319:54
leonardrbdmurray: ok, you're in the queue19:55
EdwinGrubbsleonardr: I need to get some food. Do you have any questions for me about the branch before I go?20:40
leonardredwin: no, sorry, i'm just getting started20:41
leonardri'll put questions in irc if i have them20:41
leonardredwin: i would like to see your test show the string 'Accepted invitation whilst trying to propose the team' somewhere20:51
leonardralso, 'while' should be just as good there as 'whilst'20:52
leonardrin fact that whole sentence was hard for me to parse. 'Accepted an already pending invitation while trying to propose the team for membership" might be better20:53
leonardrother than that, r=me20:53
=== EdwinGrubbs is now known as Edwin-lunch
=== leonardr changed the topic of #launchpad-reviews to: On call: leonardr || reviewing: bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardrbdmurray, can you summarize the old behavior? it sounds like bugs used to say "this bug might expire eventually" and people interpreted this as "it's time for this bug to expire"?21:07
bdmurrayleonardr: yes, that is what bug 595124 is about21:08
_mup_Bug #595124: A bug's can_expire attribute is confusing <dhrb> <easy> <Launchpad Bugs:Triaged> <Ubuntu:Invalid> <https://launchpad.net/bugs/595124>21:08
leonardrfindExpirableTasks(days_old=60) finds bugs that have not had any activity in 60 days?21:11
leonardrbdmurray -^21:11
leonardrin that case i'm confused about what isExpirable(days_old=0) means. it means that the bug can expire even if it was just changed?21:13
bdmurrayleonardr: isExpirable(days_old=0) is what can_expire used to be21:14
bdmurrayso might expire eventually21:14
leonardrthe thing i don't understand is where days_old=0 fits into that21:15
bdmurrayIt is for showing on a bug's web page that it will expire in XYZ days21:15
bdmurrayfor example bug 30102021:16
_mup_Bug #301020: Video no longer plays on S3 Savage in Intrepid <intrepid> <needs-retested-on-lucid-by-june> <xserver-xorg-video-savage (Ubuntu):Incomplete> <https://launchpad.net/bugs/301020>21:16
leonardrok, it says it will expire in 59 days21:17
leonardrso isExpirable(0) must have returned true21:17
leonardrbut i don't understand the meaning of the zero in there21:17
leonardrpresumably isExpirable(61) would have returned false?21:17
bdmurraywell 60 which is the configuration value yes21:18
leonardrwhat is the difference between isExpirable(0) and isExpirable(60)?21:18
bdmurray                    AND Bug.date_last_updated < CURRENT_TIMESTAMP21:19
bdmurray                        AT TIME ZONE 'UTC' - interval '%s days'21:19
bdmurrayit is passed to that ^ in findExpirableBugTasks21:19
leonardrso isExpirable(0) gets a bug no matter when it was updated -- will this bug eventually expire21:20
bdmurrayyes21:20
leonardrisExpirable(60) only gets bugs that have not been updated in a long time, and 60 days is the de facto point at which expirable bugs that haven't been updated _are_ expired21:20
bdmurraycorrect21:21
leonardrok21:21
leonardrin the test, you set a bug to invalid because an invalid bug won't expire?21:22
bdmurrayright otherwise I would have changed all the other tests21:22
leonardrapropos the tests you did change, i don't have enough context to know why all these bugs changed from EXPIRE=True to EXPIRE=False. which change caused that?21:23
bdmurrayswitching from "expire sometime" to "expirable" the 31 is the days old21:24
leonardrok21:24
leonardrbdmurray, in incomplete-bugs, why did you switch from a test that adds a comment to a test that directly manipulates a field value?21:28
bdmurraybecause the comment would set date_last_updated to today which is less than 60 days ago21:29
leonardrok21:30
leonardrone more question. in places like bugtask-index.pt, i assume you changed can_expire to context.bug.isExpirable(0) for clarity's sake, since the two statements are equivalent?21:30
bdmurraythey aren't equivalent because can_expire uses 60 days now21:31
leonardrok, in that case you're changing the behavior of the web service in a way that's not backwards compatible21:36
leonardrsince some bugs that used to have can_expire=True will now have False21:36
leonardrhowever, i don't think it's worth devoting a lot of effort to restoring the old behavior in old versions. flacoste, do you agree?21:37
bdmurrayokay, is there a policy about that?21:37
leonardrthe policy is to change behavior only in the 'devel' version of the web service, and maintain the old behavior in all previous versions21:39
leonardrand actually, this case might be one i didn't plan for21:40
leonardractually, nm, i have it21:40
leonardri hope flacoste will say this is not a big enough deal to maintain the old behavior, but if he says we have to maintain it, here's what to do21:40
leonardrbasically you will define two can_expire properties21:42
leonardrcan_expire and can_expire_beta21:42
lifelesshi21:42
lifelesswhats the protocol to get something reviewed - put your hand up and wait ?21:42
leonardrcan_expire will be the one you have now, but you will only publish it in version 'devel' of the web service21:42
leonardrlifeless: usually, except i'm at eod, so i won't be able to review your branch. i suggest picking a specific person who's around and asking them21:43
lifelessok21:43
leonardrbdmurray: can_expire_beta will be implemented as per the old behavior, and you will publish it as 'can_expire' starting in the 'beta' version and remove it as of the 'devel' version21:43
lifelessI'll start with a 'is there any one here up for reviewing two small refactoring branches'21:44
lifeless:)21:44
leonardrbdmurray: for an example, take a look at lazr.restful/src/lazr/restful/example/multiversion/resources.py21:45
leonardryou'll see that a_comment first shows up in version 1.0 as 'comment' -- you'll want to make can_expire_beta show up in version beta as 'can_expire', and can_expire show up in version devel21:46
leonardryou'll also see that 'deleted' is set to exported=False in version 3.0. you'll want to set can_expire_beta to exported=False in version devel21:46
leonardri will put this information into the review21:48
bdmurrayleonardr: great, thanks21:48
lifelessgary_poster: are you EOD yet?21:50
gary_posteron call21:50
lifelessdo you mean 'on a phone call' ? [conflation with 'on call' in the topic]21:50
=== leonardr 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
leonardrlifeless, he's probably on the phone21:53
leonardrflacoste, i'd like your opinion on bdmurray's branch https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/2854321:53
flacostehttps://lpstats.canonical.com/graphs/LPProjectBugTriage/21:54
lifelesstrending down21:56
lifelesssinzui: perhaps you are around and not EOD ?21:57
sinzuiI am about21:58
lifelessI wrote a couple of patches in the weekend21:58
lifelessprompted by our discussion about the milestone page performance21:58
lifelessI was following my nose into the code21:58
lifelessthey seem like improvements to me, and I'd like to get them reviewed.21:58
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/lsprof/+merge/2858321:59
lifelessand21:59
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/lognamer/+merge/2857421:59
lifelessif you have the time, some feedback would be great21:59
=== matsubara is now known as matsubara-afk
sinzuiI will review them now22:00
lifelessthank you!22:00
gary_posterlifeless: thanks for changes.  I need to run, but will look more closely later.22:13
gary_posterI misunderstood the goal of the lognamer branch (I thought you were tackling making the naming cross-process-safe) but now that I understand, cool.  I like the new explanatory comment, but I don't think an XXX (which we exclusively mean to be associated with an LP bug) is appropriate myself, because I doubt we care enough to change it.  It is more of a warning.22:13
gary_posterFor the other branch, I also like the changes on first blush.  You do have this comment: "<!-- Would be nice to turn this off in production -->".  It's possible and even easy to do that if you know the right incantation...I don't remember but lemme see if I can find it really fast...22:13
lifelessgary_poster: thank you22:14
lifelessgary_poster: I'd be delighted to make it cross process safe; the XXX I can downgrade, but it is actually unsafe at the moment, and that should be recorded - theres an expectation mismatch presently.22:15
gary_posterlifeless: Apparently there is an expectation mismatch, but I've categorized that in my mind as a documentation/understanding problem, not a code problem.  Arguable, and I won't argue it now. ;-)22:16
lifelessgary_poster: :)22:16
lifelessgary_poster: no need to argue, as you request I'll down grade it to a non-XXX note; I was trying to be clear is all22:17
gary_posterno, understood.  I would have gone along with the XXX if you really wanted too (but with a bug)22:18
gary_posterok22:18
gary_posterso, you need zcml:condition22:19
lifelessgary_poster: bugs that folk don't have time to work on are a bit close to busywork ;)22:19
gary_posteragreed :-)22:19
gary_posterin your configure element, or in an include, you say something like zcml:condition="have apidoc" except you test on the devel state...22:19
gary_postertrying to remember how to do that22:20
lifelessgary_poster: don't worry about it22:20
gary_poster./zopeappsession.zcml:10622:20
gary_posterOK22:20
gary_posteranyway22:20
lifelessgary_poster: its currently on in prod22:20
gary_posterheh22:20
lifelessremoving it is orthogonal to making it event driven22:20
gary_posterok22:20
gary_postertrue22:20
=== salgado is now known as salgado-afk
gary_posterok, running away22:20
gary_posterbye22:20
lifelessI am likely to keep pulling on this thread to get actual kcachegrind profiles22:20
lifelessciao22:20
gary_posteryay!22:20
lifelesssinzui: thanks for the reviews, I has replied.23:09
=== Edwin-lunch is now known as EdwinGrubbs
=== krkhan_ is now known as krkhan

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