/srv/irclogs.ubuntu.com/2009/12/17/#launchpad-reviews.txt

=== mwhudson_ is now known as mwhudson
mwhudsonjml: https://code.edge.launchpad.net/~mwhudson/launchpad/recipe-model-code/+merge/16272 if by some freak event you're still around08:22
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [jpds] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 checks for jpds' mp11:10
noodles775Seems to be reviewed already.11:11
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
jpdsnoodles775: ui review for the changes would be good.11:14
noodles775jpds, ok, will do!11:14
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, - || queue [jpds-ui] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado-afk is now known as salgado
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
jpdsnoodles775: Ah, that ubuntu-team thing is... interesting.12:06
jpdsnoodles775: I think we should just be able to remove people silently as the bug report says, but adding should still notify people.12:07
jpdsHave to fix the template now.12:07
jpdsIs there a way of having multiple tal:condition='s in one <span> ?12:16
adiroibanjpds: nope. but you can use python to handle complex conditions12:17
adiroibantal:condition="python: PYTHON WONDERS LAND"12:18
adiroibanand view/something is view.something12:18
jpdsadiroiban: Neat.12:25
jpdsnoodles775: Thanks for the review, I've fixed the issues you pointed out.12:25
=== adiroiban changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, - || queue [adiroiban(bug-492375)] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* maxb would like to enqueue https://code.edge.launchpad.net/~maxb/meta-lp-deps/no-sshd/+merge/1628012:51
=== maxb changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, - || queue [adiroiban(bug-492375), maxb#495454] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775jpds: regarding always notifying on enabling - yes, I think you're right. I was thinking specifically about the case where:13:00
noodles775an admin silently deactivates a membership, but then needs to re-enable it as it was a mistake, but in that case, there's no need to do it silently - a mistake can be admitted :)13:01
noodles775jpds: and thanks for implementing the other suggestions!13:01
jpdsnoodles775: Interesting situation, althought I'd imagine that this feature would be used with caution.13:07
jpdsnoodles775: Pushed final changes, you can land if you're happy with the changes.13:09
noodles775jpds: thanks! I'll leave the landing to bac when he re-reviews the code changes.13:10
jpds\o/ Awesome.13:11
noodles775maxb: sorry, I'm not so familiar with the lp dependencies - is there a reason why you've not incremented the version to 0.62 and added a new changelog entry, rather than editing the 0.61 entry that you created with r67?13:18
noodles775adiroiban: thanks for the details in your MP. Unless I hear from sinzui (he may want to take it) I'll start a UI review in an hour or so, and leave the code to al-maisan.13:21
adiroibannoodles775: sure. there's no hurry13:22
jpdsHmm, now that the rollout is done, does that mean that we can land https://code.edge.launchpad.net/~jpds/launchpad/fix_176396/+merge/16082 ?13:28
maxbnoodles775: Because 0.61 hasn't been built/uploaded yet13:29
noodles775maxb: ah thanks. I've already approved it.13:29
leonardrsalgado: can you spare a couple minutes to help me wrap up the launchpad anonymous-access branch?13:46
salgadoleonardr, sure, what's up?13:46
leonardrsalgado: http://pastebin.ubuntu.com/343545/13:47
leonardri made a tarball of launchpadlib 1.5.4 (pre-release) and changed launchpad to use it13:47
leonardri ran the tests last night and they all passed13:47
leonardrso if you'll approve that branch i can land it13:47
salgadosure, looks good to me, but you won't be able to land it as pqm is closed13:48
leonardrargh. i always lose track. when does it reopen? next year? and how can i find that information as someone who only does a launchpad branch very occasionally?13:50
maxbnoodles775: What's the proper [r=] tag for you? I had assumed it was LP username, but I see you're michael.nelson there, but I'm fairly sure I've seen [r=noodles] in the past13:51
noodles775maxb: yeah, noodles is it.13:52
maxbgah, and after asking that, I got carried away with debcommit and forgot to add it14:04
al-maisannoodles775: how does the ui/code review split work?14:24
noodles775al-maisan: you just do a normal code review.14:25
al-maisannoodles775: OK, thanks!14:25
=== al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, adiroiban(bug-492375) || queue [maxb#495454] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
al-maisanhello adiroiban : just making sure I understand things properly: should I see the "View all languages" link on https://translations.launchpad.dev/ubuntu when I am *not* logged in?14:35
adiroibanal-maisan: yes14:36
al-maisanadiroiban: pebkac, I started looking at the page prior to merging your branch :P14:38
al-maisansorry14:38
adiroibanal-maisan: well... you lost me :)14:38
adiroibanah.14:39
al-maisanadiroiban: don't worry .. I just need to merge your branch ..14:39
adiroibangoogle put me back on track :)14:39
al-maisanadiroiban: based on geoip the following languages are offered to me while I am logged off: afrikaans; sotho, southern; xhosa, zulu14:42
al-maisanthat's a bit funny since I am based in DE14:43
adiroibanyep...14:43
adiroibanI did not changed that code14:43
al-maisanadiroiban: no problem -- just commenting on it..14:43
adiroibanadiroiban: yep. I know. Jeroen told me they are thinking to remove that feature14:44
adiroibanstill, in the current code I'm just using the list14:44
al-maisanadiroiban: the page you described in the "Demo and Q/A" works as expected .. now on to the code14:50
adiroibanhooray14:50
al-maisanI am getting this error when I run a windmill test:15:00
al-maisan  Set up lp.translations.windmill.testing.TranslationsWindmillLayer <type 'exceptions.KeyError'>:15:00
al-maisan'MOZILLA_BINARY'15:00
al-maisanany idea what it means?15:01
al-maisanthis is the full paste: http://pastebin.ubuntu.com/343602/15:01
leonardrnoodles776 or al-maisan, can you put a quick contrib branch in the queue?15:02
leonardrhttps://code.edge.launchpad.net/~rick-rickspencer3/lazr.restfulclient/add_gaurd_for_None_to_eq_functions/+merge/1604315:02
adiroibando you have firefox installed in the right location?15:02
leonardrreview my revised diff, not the original branch. (i can push another branch if you want, but i'd like to keep the review here)15:02
al-maisanadiroiban: ah, that explains it, I don't have firefox installed at all -- this is run in a chroot15:03
* al-maisan installs firefox15:03
adiroiban:)15:03
adiroibanThis is my first windmill test15:03
al-maisannoodles775: could you take the branch that leonardr asked about .. I'll need to attend the other meeting in an hour..15:03
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: jpds-ui, adiroiban(bug-492375) || queue [adiroiban-ui, maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: -, adiroiban(bug-492375) || queue [adiroiban-ui, maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775al-maisan: i've put it in the queue for the moment, so either I or one of the US reviewers will get to it :)15:05
al-maisannoodles775: good man :) thanks!15:05
noodles775leonardr: I don't understand - haven't you just reviewed that LAZR branch? or at least asked for changes (extra tests)?15:07
leonardrnoodles775: i pointed out that there were no tests, and went ahead and added the tests because rick isn't on irc and i figured that was the best way to show him how the test system worked15:09
leonardrbut since my code is now in the branch i don't want to approve my own code15:09
=== salgado is now known as salgado-lunch
noodles775leonardr: right - I was confused because the revision with your tests wasn't yet showing on the branch (on the MP).15:10
noodles775Still isn't, but I assume it'll appear soon :)15:10
leonardrnoodles: i don't think i can push to the branch--it's rickspencer3's branch15:11
leonardrthat's why i offered to push a different branch, but said i'd prefer that the review happen in the original15:11
noodles775leonardr: Sorry - I should read a bit more thoroughly before asking questions. OK, I'll review it with your tests and assume they'll be included.15:12
leonardryeah, my branch is what i'll merge15:12
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: adiroiban-ui, adiroiban(bug-492375) || queue [maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== jamalta_ is now known as jamalta
al-maisanadiroiban: this windmill test fails on my system (maybe due to my strange setup):15:16
al-maisantest_results: ERROR    Test Failure in test {'version': '0.1', 'suite_name': 'SeriesLanguages Tables', 'result': False, 'starttime': '2009-11-17T20:43:5.858Z', 'output': None, 'debug': 'Looking up name loginpage_submit_login, failed.', 'params': {'name': 'loginpage_submit_login', 'uuid': 'acee01d8-eb1e-11de-a198-00218652d375'}, 'endtime': '2009-11-17T20:43:5.859Z', 'method': 'asserts.assertNode'}15:16
adiroibanal-maisan: ah... that is part of user.ensure_login(self.client)15:17
adiroibanand it fails for all windmill tests15:17
al-maisanadiroiban: good to know :) thanks!15:17
adiroibanfrom canonical.launchpad.windmill.testing import lpuser15:18
adiroibanand I'm using user = lpuser.TRANSLATIONS_ADMIN15:18
adiroibanto get a user for which I know the preferred languages15:19
adiroibanI think the windmill test are still very fragile15:19
adiroibanfor translations15:19
adiroibanare are only 2 or 3 other windmill tests15:19
al-maisanadiroiban: so, I am wondering: will this also fail after the branch is reviewed and submitted to PQM?15:20
adiroibanal-maisan: the translations Windmill test are disabled in pqm15:20
adiroibantests15:20
al-maisanadiroiban: ah, yes, that's right!15:21
al-maisanthanks again!15:21
adiroibannp15:21
al-maisanadiroiban: you added isPreferredLanguage() twice .. is there any particular reason for that?15:29
adiroibanit should be an error 15:30
adiroibanlet me check15:30
adiroibanon is for distroseries15:30
adiroibanand the other is for productseries15:30
adiroibanbut that should be all15:30
al-maisanyes15:30
al-maisanwas there no good place to put it once and reuse it?15:31
adiroibanno15:32
adiroibanwe talked about that in the pre-implemetnation call15:32
al-maisanaha15:32
adiroibanin the future we should have a single serieslanguage view15:32
adiroibanfor both product and distro15:32
adiroibanright now also preferred_languages15:32
adiroibanimplements the same logic15:34
adiroibanin both classes15:34
adiroibani think there is a bug about than creating the interface... and if not, I will add it now15:35
al-maisanadiroiban: cool .. please provide me with the bug number.15:37
* adiroiban is searching 15:37
adiroibanbug 49636115:38
mupBug #496361: Register a single view (SeriesLanguageView)  for all ISeriesLanguage-implementing objects <Launchpad Translations:New for adiroiban> <https://launchpad.net/bugs/496361>15:38
al-maisanadiroiban: thanks !15:38
adiroibanbut there is a lot of refactoring work in there 15:38
adiroibanso maybe after analyzing it, it should be splited in other bugs15:39
al-maisansure, no problem15:39
al-maisanadiroiban: there is one little typo (please see the review email), otherwise r=me15:40
adiroibanal-maisan: thanks. I'll fix it and push the changes right away... anyway we can not land it now15:41
=== al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: adiroiban-ui, <meeting> || queue [maxb#495454, leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
adiroibanal-maisan: i don't see any info about the typo15:45
noodles775adiroiban: ok, I've just sent the ui review too... great work!15:45
al-maisanadiroiban: just a sec15:45
adiroibansure15:45
al-maisan+ * Set up initial the visibility for languages in a serieslanguages table.15:46
al-maisanshould probably read:15:46
al-maisan"Set up the initial visibility.." ?15:46
al-maisanadiroiban: ^15:46
adiroibanyes. sorry for that. 15:46
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: maxb#495454, <meeting> || queue [leonardr] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
al-maisanadiroiban: take it easy :) erring is human, and, your branch looked great otherwise.15:47
noodles775Sure did/does!15:48
=== noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: leonardr, <meeting> || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
al-maisannoodles775, adiroiban: my review email appeared with some delay but it's there now.15:50
adiroibanal-maisan: ah. ok. np15:50
adiroibannoodles775: regarding word capitalization. I agree15:58
adiroibanbut I tried to be consistent with the previous implementation15:58
adiroibanhttps://translations.edge.launchpad.net/ubuntu/karmic/+source/gfxboot-theme-ubuntu/15:59
adiroibanso then I should also open a bug for that15:59
adiroiban?15:59
noodles775adiroiban: yeah, I saw that you'd kept it consistent. I don't think it's worth a bug to fix the capitalization - it's easy enough to do little drive-by cleanups like this when the page is touched. Up to you.16:00
adiroibannoodles775: ok. I will change it in this branch16:01
noodles775Great, thanks!16:01
=== salgado-lunch is now known as salgado
adiroibannoodles775: I will also hide the „Show all languages” when not needed. I was thinking that most translators/visitors will define their preferred languages and I was avoiding to much logic in that view16:05
noodles775adiroiban: ah, is "English" the default if a person has never set their preferred language? I didn't realise that. But yes, there could be a lot of people in that situation.16:08
noodles775(I mean, as in "English" is actually selected, as opposed to not having any preferred language selected).16:08
salgadoadiroiban, did you run the test suite after doing the test changes you just added to https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994?16:11
adiroibansalgado: is running16:13
adiroibannot finished yet16:13
salgadoadiroiban, I think it won't work because your changes expect view.initialize() to return the view, but it doesn't16:13
adiroibansalgado: true16:16
adiroibansalgado: i need to do self.view.initialize()16:17
salgadoright16:17
=== al-maisan changed the topic of #launchpad-reviews to: on call: noodles775, al-maisan || reviewing: leonardr, - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
salgadoadiroiban, it may be that you don't even need to instantiate the view again -- just calling .initialize() on it when you update stuff might be enough16:18
adiroibansalgado: indeed ... now testing16:20
adiroibansalgado: is working for @property but not for @cachedproperty16:24
salgadoright, that's expected.  I'd forgotten there was a cached property16:24
adiroibanI'll push the changes and start a full test using that code16:26
=== noodles775 changed the topic of #launchpad-reviews to: on call: al-maisan || reviewing: - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || RC candidates take priority || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
leonardrgary: https://code.edge.launchpad.net/~leonardr/launchpadlib/use-shorthand-uris-in-all-tests/+merge/1629817:05
leonardrthe diff is here if you need it:17:06
leonardrhttp://pastebin.ubuntu.com/343631/17:06
gary_posterleonardr: on it17:07
gary_posterleonardr: my only thought is that it might be nice to explain what that test_dev string is in introduction.txt.  Agree?17:10
gary_poster(and where it is defined)17:12
leonardrgary, sure17:19
gary_posterleonardr: ok, will merge-conditional on that then17:20
leonardrgary: it's defined in uris17:21
gary_posterleonardr: yeah, I saw that in the diff.  I meant that it would be nice to have an explanation in the text17:22
leonardrsure17:22
leonardrgary: ok, you should be able to update launchpadlib and do a release now17:37
=== fjlacoste is now known as flacoste
gary_posterleonardr: http://paste.ubuntu.com/343640/18:09
leonardrwtf, how did that test not get run18:10
leonardrit looks like they haven't been run for a while18:10
leonardrthat first failure looks like it was caused by a change landed while i was on vacation18:10
leonardrgary: ok, the problem is that testing launchpadlib in a launchpad context only runs the doctests, not the unit tests18:14
gary_posterleonardr: ah ok18:14
leonardri and others have only been running the doctests. i'd like to fix launchpad to run the unit tests too, but i'll settle for fixing the tests for now18:14
gary_posterfair enough18:16
gary_postermaybe add a bug for getting launchpad to run the unittests?18:17
leonardryeah18:18
leonardrif allenap were around i'd have a better shot at fixing this stuff, he landed the two branches18:18
gary_posterurg. :-(18:18
gary_posteryeah, the last one was the only one that clearly pointed to your recent changes18:19
leonardrgary: there's a chance it might be more fallout from my login_With refactoring18:24
gary_posterleonardr: ok.  You agree that we ought to give you a chance to fix these before making a release?18:24
leonardrgary: yes18:25
gary_postercool18:25
leonardrgary: https://code.edge.launchpad.net/~leonardr/launchpadlib/test-failures/+merge/1630318:49
gary_posterleonardr: approved (sorry was on calls)19:45
gary_posterleonardr: going to lunch then will make release19:45
leonardrgary: np19:45
leonardrall right19:45
abentleyIs there a team for UI reviewers?20:12
=== salgado is now known as salgado-afk
gary_posterleonardr: if you make the comment fix and merge then I can make the release20:18
leonardrgary: i'm having trouble committing20:18
leonardrhttps://bugs.edge.launchpad.net/bzr/+bug/246233 seems to be happening to me20:18
mupBug #246233: TooManyConcurrentRequests error when ssh connection fails (bzr crashes when pulling) <hpss> <5-a-day-data:Invalid> <Bazaar:Fix Released by spiv> <https://launchpad.net/bugs/246233>20:19
leonardrok, it seems to have been transient20:19
leonardrgary: commited now20:19
gary_postercool thanks20:19
allenapgary_poster, leonardr: Hi, I did run the test suite before landing those branches ;) There was one failing test which I filed a bug for iirc, but obviously it's not good now.20:21
leonardrallenap: it turned out to be my fault20:22
gary_posterleonardr: did you see my comment about line 39 in the diff?20:22
allenapleonardr: Oh, phew :)20:22
gary_posterallenap: what he said. :-) thanks for saying hi though20:22
gary_posterleonardr: "be returned as is).  :param launchpadlib_dir: The user's" should have a \n in there20:23
leonardrgary: missed that, i'll fix it20:23
gary_posterthanks20:23
leonardri also fixed a typo that broke a test, but i think i fixed that before you saw the branch20:23
gary_posteryes you did20:23
gary_posterthe review UI is imporving nicely20:24
gary_posterimproving20:24
leonardrgary: i also added instructions on developing lazr.* branches and getting them landed to https://dev.launchpad.net/HackingLazrLibraries20:25
gary_posterleonardr: I saw that in your email to jml but had not read the content.  Looks great, thank you.20:26
leonardrgary: ok, formatting is fixed20:29
leonardrhopefully the release will work now and i can give you the stuff to land my launchpad branch once pqm opens20:29
gary_posterleonardr: everything worked.  going to pick up son.20:42
leonardrgary: ok, i'll make instructions for you20:42

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