/srv/irclogs.ubuntu.com/2010/09/23/#launchpad-reviews.txt

=== Ursinha is now known as Ursinha-afk
wallyworld_any on-call reviewers around?04:53
mwhudsonwallyworld_: i only play one on tv05:07
mwhudsonwallyworld_: link me up?05:07
wallyworld_:-) here it comes...05:07
wallyworld_https://code.edge.launchpad.net/~wallyworld/launchpad/recursive-branch-resolution-failure/+merge/3640305:07
wallyworld_thhanks in advance05:07
mwhudsonwallyworld_: http://launchpadlibrarian.net/56281944/Screenshot-1.png is pretty funny :-)05:10
wallyworld_yes, except if it was your code that introduced the behaviour :-/05:11
mwhudsonwallyworld_: do you think you can change the name of the 'url' variable in redirect_branch ?05:39
mwhudsonwallyworld_: perhaps to referer or http_referer ?05:40
wallyworld_sure. how about "mary"?05:40
wallyworld_:-)05:40
wallyworld_ok.05:40
wallyworld_that's a better name for sure05:40
mwhudsonwell, meaningless *might* be better than confusing....05:40
mwhudsonotherwise fine05:40
* mwhudson goes to do the clicky clicky05:40
wallyworld_cool. thanks.05:40
wallyworld_mwhudson: btw, tim let the url variable name through the first time around :-)05:41
mwhudsonhm, really redirectSubTree(None) should explode05:41
wallyworld_yeah, on firefox and chrome it does05:41
mwhudsoni mean serverside05:41
wallyworld_not sure, haven't looked into that code path05:42
mwhudsonit's clearly a programming mistake05:42
mwhudsonwallyworld_: it seems like it redirects to +branch/None :)05:42
wallyworld_yes, it does, and then it comes back into the redirect method and so on05:42
wallyworld_chrome/firefox after a while complain hat the target url can never be reached05:43
mwhudsonright05:43
wallyworld_not sure what i did but it took me lots of stuffing around to even be able to get the screenshot attached to the bug report to show up05:43
wallyworld_so i just looked at the code and guess the problem05:44
mwhudsonwhat i am saying is that calling redirectSubtree(None) should raise, say, AssertionError05:44
wallyworld_ack.05:44
mwhudsonwhich would result in an OOPS05:44
wallyworld_+105:44
mwhudsoni guess i can file that as a bug05:44
wallyworld_want me to do a drive by?05:44
wallyworld_on second thoughts, too much other stuff could potential be affected05:44
mwhudsonwallyworld_: not really05:45
wallyworld_best to do a new bug05:45
wallyworld_mwhudson:  i'll fix the variable and land it. thanks for the review05:45
mwhudsonwallyworld_: np05:46
mwhudsonwallyworld_: i filed https://bugs.edge.launchpad.net/launchpad-foundations/+bug/645751 btw05:48
_mup_Bug #645751: Navigation.redirectSubTree(None) should OOPS <Launchpad Foundations:New> <https://launchpad.net/bugs/645751>05:48
wallyworld_mwhudson: thanks. i looked at the variable name and it should not be called http_referer. it is actually the target url that will be redirected to. how about then "target_url"05:49
wallyworld_it is just initially set to the refer05:50
wallyworld_i can add a comment in the code if you like as well05:50
mwhudsonoh05:50
wallyworld_just to make it explicitly clear05:50
mwhudsonmaybe then a comment to say that the only way it can be None is if the resolution failed _and_ there was no referrer?05:50
mwhudsonby the "is None" check05:50
wallyworld_yes, ok. i think the main thing missing was some code comments to explain things05:51
wallyworld_i mentioned stuff in the merge proposal but not the code05:52
wallyworld_mwhudson: ping?06:12
mwhudsonwallyworld_: hi06:12
wallyworld_there was a code path that i missed out06:13
wallyworld_if the branch was private, it could still redirect to a None url06:13
wallyworld_i fixed the code and added a new unit test06:13
wallyworld_can you please take a quick look?06:13
mwhudsonwallyworld_: ok06:14
wallyworld_thks06:14
mwhudsonwallyworld_: have you pushed the branch?06:15
wallyworld_yep. lp may be having a little snooze06:15
mwhudsonwallyworld_: i bet the fact that bazaar.launchpad.net:22 has fallen over is stopping you06:15
wallyworld_bugger06:15
mwhudsonwallyworld_: it's back now06:16
wallyworld_ack06:16
wallyworld_mwhudson: pushed this time. i didn't notice the error last time.06:20
mwhudsonwallyworld_: looks fine06:24
wallyworld_excellent. thanks!06:24
* mwhudson goes for the bus06:24
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jmlcan I get a quick review for https://code.edge.launchpad.net/~jml/launchpad/better-slave-mock/+merge/3641709:10
noodles775Sure09:11
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jmlnoodles775, thanks.09:24
noodles775jml: all good, r=me10:07
jmlnoodles775,10:08
jmlnoodles775, cheers.10:08
lifelessnoodles775: hi10:09
noodles775Hi lifeless10:09
lifelessnoodles775: could you please try reenabling that test.10:09
lifelessit was fixed, on devel, I'm positive.10:09
noodles775jml: did you get to check the rev of devel when your branch failed ec2?10:10
jmlnoodles775, no, I didn't.10:10
jmlnoodles775, no one gave the revno of the threatened fix, and things are pretty hectic around here10:11
lifelessthere is a subtle problem in oops with getLastOopsReport; that test didn't need to use it, and my branch which is landed fixed that.10:11
lifelessI know it fixed it because it was landed *with* a branch suffering the problem.10:11
noodles775jml: if you can forward your ec2 failure email, I'll happily chase it up.10:12
noodles775lifeless: sure, I'm happy to re-enable it, I just want to check the revs. Which rev was your fix?10:13
lifelessdunno, I just know I got the 'its merged' mail.10:13
jmlnoodles775, http://paste.ubuntu.com/497557/ ; http://paste.ubuntu.com/497555/10:13
lifeless1159710:14
noodles775Great, re-enabling now... thanks jml, lifeless10:14
leonardrnoodles775, yesterday i got a ui review from edwin. i'm making his suggested changes, and then i'd like to put my branch in the queue for a ui/code review from you13:15
noodles775leonardr: sure - one of us will take a look (I think allenap will be around too).13:16
leonardrthanks13:16
noodles775leonardr: actually, if you mean the rename-grant-permissions branch, I think lifeless was keen to have a lot more discussion on the dev list about it? (See comments on the MP).13:19
leonardrindeed, i was looking right at it13:20
noodles775Great.13:20
noodles775leonardr: the need for discussion about those changes came up earlier today on irc (lp-dev) - you can find it in the irclogs (search for your mp url) in case it's useful.13:21
=== mrevell is now known as mrevell-lunch
=== noodles775 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
=== Ursinha-afk is now known as Ursinha
=== mrevell-lunch is now known as mrevell
=== 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
bachi allenap, i have a branch that is super-sized with a diff of 1432 lines.  can you review it?14:27
allenapbac: Yeah, sure. I'm just having a look at jam's branch right now, deciding if I should take it on.14:27
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: jam || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bacallenap: ok, thanks.  https://code.edge.launchpad.net/~bac/launchpad/bug-643538-code/+merge/3637714:28
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
allenapbac: I get conflicts merging into devel.14:32
bacallenap: oh, sorry.  let me fix that14:33
allenapbac: It's fairly trivial I think.14:33
allenapbac: It merges cleanly into the very latest devel. Sorry, I last pulled it a few hours ago.14:49
bacallenap: really?  i got conflicts.14:49
baci've merged and am pushing it up now14:50
allenapbac: Weird!14:50
bacallenap: just pushed r 1152814:51
noodles775salgado: Hi! I was going to ask henninge - but he's not available. Are you up for a pre-lim. UI review?14:57
noodles775https://code.edge.launchpad.net/~michael.nelson/launchpad/644328-blacklist-ui/+merge/3644214:57
salgadonoodles775, sure!15:02
noodles775Thanks salgado15:07
james_wnoodles775: very nice15:12
noodles775Thanks james_w :)15:12
james_wnoodles775: that's all for real?15:12
noodles775james_w: yep - you can run it with the instructions on the MP. But there's still lots missing (like the actual generation of the diffs etc.).15:14
salgadonoodles775, what does blacklisting do here?15:16
james_wnoodles775: that's great15:16
james_wnoodles775: thanks for your work on this15:16
noodles775james_w: np - I'm enjoying the UI work :)15:17
noodles775salgado: it allows you to hide differences that you don't care about in your derived distroseries.15:17
* noodles775 checks to see if the LEP has a better description.15:17
noodles775salgado: see the section 'Marking a package as blacklisted/ignored in the thread at: https://lists.launchpad.net/launchpad-dev/msg04536.html15:20
salgadooh, that clarifies things.  I was going to ask what's the difference between blacklisted and ignored (which is what I see in one of the mockups)15:21
salgadonoodles775, so, when you blacklist all versions of something, they show up at +missingpackages?15:22
noodles775salgado: no - missing packages is just for packages that are in the parent series, but not the derived. You will see the blacklisted items on this same page, but only when you select the checkbox in the filter options ('include blacklisted packages' on the mock, not yet implemented).15:24
noodles775bug 64129015:24
_mup_Bug #641290: Add search for DistroSeriesDifferences <derivation> <Soyuz:Triaged> <https://launchpad.net/bugs/641290>15:24
salgadonoodles775, this may not be pertinent, but have you thought about what the "This difference has been blacklisted ..." text will look like for blacklisted packages when the search allows them to be shown?15:34
noodles775salgado: Yes, a little. Part of the reason for graying them out and using that text was that it would/should be consistent with results when searching. Can you see a situation or reason why it would need to be different?15:36
salgadonoodles775, I like the graying out because it makes it easy to see everything that was changed at a glance and maybe undo, but the text (as well as the links) is a bit too verbose (IMHO) and don't allow switching between all different states, so I was thinking of using 3 radio buttons there.  "Blacklisted: (*) No,  ( ) This version,  ( ) All versions"15:39
salgadobut that's more like a nice-to-have thing, not that important15:40
noodles775salgado: that actually sounds a bit like what I'd proposed on the mockup (a single 'Add to blacklist' that would then give you those options), but henninge convinced me on a previous MP that it was a bit too much... hangon, I'll find the discussion.15:42
salgadowell, except that the radio buttons allow you to see all the possible states without having to click anything15:43
salgadojust like the links do15:43
salgadobut sure, maybe he'll be able to convince me as well. :)15:43
noodles775salgado: ok - if you search for blacklist at https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 you'll see a bit of background.15:46
noodles775salgado: but I think your radio button option you mention could work too... Also, if you've got suggestions for making the text less verbose while still being easy to understand, let me know :)15:48
noodles775s/you/that you/15:48
salgadowhat I like about the radio button is that you see all the options without having to click anything and there's no repetition15:49
salgadothe downside is that you need vertical real estate, but you seem to have plenty of that15:49
salgadoas for the text of the link, the only thing I can think of is something like "Blacklist: _this version_ or _all versions_"15:51
salgadobut I don't quite like that as it seems to suggest you need to blacklist something15:51
=== benji changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
benjimy MP is at https://code.edge.launchpad.net/~benji/launchpad/wadl-refactoring/+merge/36452; should be a quick one15:55
noodles775salgado: Yeah. RE the radio buttons, I was going to say that it would require two clicks (ie select/submit), but it doesn't of course... we could do the api update when an option is selected quite easily. And that would mean no need for 'Undo'. I like it more and more :)15:56
salgadoindeed, I hadn't thought about the undo15:58
abentleyEdwinGrubbs, I had to tweak my branch slightly since you reviewed it.  Do you mind taking another look? https://code.edge.launchpad.net/~abentley/launchpad/fix-groupby/+merge/3636016:04
=== deryck is now known as deryck[lunch]
salgadonoodles775, I think that's the only suggestion I have.  shall I paste our chat there for future reference?16:11
noodles775salgado: yeah, that'd be great, thanks.16:11
salgadonoodles775, np.  glad you liked my suggestion. :)16:13
noodles775I do indeed!16:13
salgadonoodles775, if I can make another suggestion (this one not UI related), you could do a 'make newsampledata' after running http://pastebin.ubuntu.com/494656/ and merge just those changes to make it easier to play with that page on lp.dev in the future. :)16:21
noodles775salgado: you'd prefer that? I thought we wanted to get rid of sample data (maintaining it etc.), and we can make it a one-liner to add the demo data?16:22
salgadonoodles775, we want to get rid of the test sample data.  if you run that script and run 'make sampledata', you'll be modifying just the demo data. :)16:23
salgadothey're separate16:23
salgadowhen you run 'make sampledata', the changes done to launchpad_dev end up in the demo data and the ones done to launchpad_ftest end up in the test sample data16:24
noodles775Yep - makes sense..., but currently that flag adds a feature flag too, do you think that's valid for the sample data? I'd think the dev server should have the same features as production by default?16:25
noodles775s/that flag/that script/16:26
salgadothat's a good question. I'd be in favour of enabling everything in .dev, but I can see why that may not be desired16:27
bacallenap: have time for another, very straightforward one?  or are you sick of me?16:44
bacallenap: it is for bug 64455016:44
_mup_Bug #644550: Robots can index blueprints when it is not used <bridging-the-gap> <Launchpad Blueprints:In Progress by bac> <https://launchpad.net/bugs/644550>16:44
=== bac changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [benji, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
allenapbac: Yeah. I'm nearly finished on your first branch, so sure.16:45
bacallenap: thanks.16:45
allenapUnless benji's is *huge* :)16:45
bacallenap: but, alas, i must step out, so feel free to skip if you have questions.16:45
benjinah, my branch is small and in fact all the code has been through review once before16:46
allenapCool.16:46
allenapbac: No worries.16:46
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: benji || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== deryck[lunch] is now known as deryck
=== benji is now known as benji-lunch
=== Ursinha is now known as Ursinha-lunch
=== matsubara is now known as matsubara-lunch
=== jcsackett changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: benji || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jcsackettallenap: MP for code review is here: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-translations-service-643545-0/+merge/3646417:16
allenapjcsackett: I'm probably not going to get to it today, sorry :-/17:16
jcsackettallenap: that's fine. i'll find someone else then. just saw you in the on call section. :-)17:16
* jcsackett has yet to figure out when people's days end.17:17
allenapjcsackett: Cool. I would do it, but I'm near my EOD.17:17
jcsacketti gathered. as i said, no worries, allenap. have a good evening. :-)17:17
allenapYou too :)17:18
=== salgado is now known as salgado-lunch
=== allenap changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: bac || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
allenapbac: I'll be back online in ~3 hours. I can review your branch then if someone else hasn't gotten there first.17:37
=== allenap changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== gary_poster is now known as gary-lunch
jcsackettanyone available for a code review?17:42
jcsackettnm. i'll ping again in a few.17:45
=== jcsackett is now known as jcsackett|lunch
=== benji-lunch is now known as benji
bacallenap: thanks for the review!18:27
bachey rockstar, you reviewing today?18:29
rockstarFUUUUUUUUUUUUUU...18:29
rockstarbac, yes18:29
* rockstar hangs head in shame18:29
=== gary-lunch is now known as gary_poster
=== rockstar changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [bac, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bac:)18:29
=== rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
rockstarbac, your review got done?18:30
bacrockstar: one did, one didn't18:30
bacthe one that needs doing is pretty trivial18:30
rockstarbac, okay.  Lemme do jcsackett's really quick, and then I'll do yours.18:30
bacno rush18:31
=== bac changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: jcsackett || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
rockstarjcsackett|lunch, thanks for PEP8ing tales.py18:35
rockstarjcsackett|lunch, so configure_translations and configure_blueprints head to the same URL?  That doesn't make much sense to me.18:44
=== salgado-lunch is now known as salgado
EdwinGrubbsabentley: I'll look at it now.18:51
rockstarjcsackett|lunch, why did you remove the Copyright header in lib/lp/translations/tests/test_hastranslationtemplates.py18:54
EdwinGrubbsabentley: that looks good18:55
EdwinGrubbsabentley: r=me still18:55
abentleyEdwinGrubbs, thanks.18:55
=== jcsackett|lunch is now known as jsackett
jsackettrockstar: copyright notice was a goof, corrected.19:01
jsackettconfigure_blueprints and configure_translations are both links to edit, which is the place to set up both of those fields. configure_blueprints was added in the unknown-blueprints branch so that the template could be relatively consistent; the same intent and method was borrowed for this branch with configure_translations.19:03
jsackett(this is for the distribution, on product they point to different spots).19:03
jsackettrockstar ^19:03
rockstarjsackett, okay.19:03
rockstarjsackett, I think this looks good.  Thanks for the patch.19:03
jsackettrockstar: thanks for the review. :-)19:04
=== matsubara-lunch is now known as matsubara
=== Ursinha-lunch is now known as Ursinha
leonardrrockstar, trivial review coming up for you20:15
rockstarleonardr, ack20:15
=== rockstar changed the topic of #launchpad-reviews to: On call: rockstar || Reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardrrockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/remove-26ism/+merge/3649320:27
rockstarleonardr, great.  I'll take a look when I'm off the phone.20:27
jsackettsalgado: ping20:34
salgadohi jsackett20:34
jsacketthi, salgado. sinzui has once again directed me to you for round one of ui review on an MP20:35
jsacketthttps://code.edge.launchpad.net/~jcsackett/launchpad/unknown-translations-service-643545-0/+merge/3646420:35
jsackett(this time there's sample data that shows the setup :-P)20:35
jsackettif you could take a look at it sometime in the near future, salgado, i would appreciate it. no super rush--i'm not looking to land it today.20:36
bacthanks rockstar20:38
salgadojsackett, oh, sample data is cool, thanks.  I may not get to it today, but in the worst case I'll review it tomorrow20:39
jsackettsalgado: sounds good. thanks!20:39
salgadodev sample data is cool, I mean20:39
jsackettsalgado: yes. the testing kind is somewhat irritating. :)20:39
salgadojsackett, you're being kind.  but I guess you put it that way because you haven't been here for too long so they haven't caused you that much pain. ;)20:41
jsackettheheh.20:41
=== rockstar 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 is now known as salgado-afk

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