| al-maisan | Good morning! | 06:17 |
|---|---|---|
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| === henninge_ is now known as henninge | ||
| jtv | henninge: care to review? | 09:56 |
| jtv | https://code.edge.launchpad.net/~jtv/launchpad/bug-440445/+merge/12777 | 09:56 |
| henninge | jtv: so you are assuming that it will also need access to language and translator? | 10:03 |
| jtv | henninge: yes; it's looking at TranslationGroup to check for membership, and that definitely involves Translator. | 10:04 |
| henninge | jtv: valid assumption, I guess. | 10:04 |
| henninge | jtatum: r=me | 10:05 |
| jtv | Not sure about Language, but it's one of those things that are likely to be fetched by the ORM as a side product | 10:05 |
| jtv | henninge: no, the other person. :) Thanks. | 10:05 |
| henninge | jtv: r=me | 10:05 |
| jtv | :) | 10:05 |
| henninge | jtatum: whatever branch you want reviewed, I have *not* approved it! ;-) | 10:05 |
| henninge | jtatum: sorry to bother you ... | 10:05 |
| henninge | jtv: please remember to set the mp to "approved". For some reason I cannot do it. | 10:07 |
| jtv | henninge: sure, thanks | 10:07 |
| al-maisan | hello adeuring -- could you please have a look at a small/simple branch? | 10:19 |
| al-maisan | it is here: https://code.launchpad.net/~al-maisan/launchpad/cmsg-425800/+merge/12774 | 10:19 |
| adeuring | al-maisan: I'm already doing this :) | 10:19 |
| al-maisan | adeuring: ah, wow! | 10:19 |
| al-maisan | adeuring: I am duly impressed :) | 10:20 |
| adeuring | al-maisan: there are two new tests: test_requestMirror_doesnt_demote_branch() and test_requestMirror_can_promote_branch(). Perhaps I'm missing something obvoius there -- but isn't future_time in the latter test a timestanp in the past: future_time = datetime.now(pytz.UTC) - timedelta(days=1) ? | 10:29 |
| * al-maisan looks | 10:29 | |
| al-maisan | adeuring: argghh | 10:30 |
| al-maisan | the diff generated for the MP is wrong | 10:30 |
| al-maisan | just a minute | 10:30 |
| al-maisan | adeuring: http://pastebin.com/m4c94727b | 10:31 |
| al-maisan | sorry about that | 10:32 |
| al-maisan | this is a *really* simple branch | 10:32 |
| al-maisan | bbiab | 10:32 |
| adeuring | al-maisan: Ah, indeed quite much shorter ;) | 10:32 |
| adeuring | al-maisan: but the other test is there nevertheless ;) | 10:32 |
| al-maisan | adeuring: aha | 10:33 |
| al-maisan | adeuring: I don't see any of that in the diff I provided you with | 10:34 |
| * al-maisan rubs his eyes | 10:34 | |
| adeuring | al-maisan: what I menat: I am simply curious about this test, even it is is not part of the patch. | 10:34 |
| al-maisan | adeuring: yep, 'future_time' would have to lie in the past .. provided the time on the test machine is set correctly :) | 10:35 |
| * adeuring is quite confused... future time in the past? | 10:37 | |
| adeuring | al-maisan: but let's put that aside. | 10:37 |
| adeuring | al-maisan: al-maisan: just one question about the short diff: You removed the text "Did you mean to upload to a PPA?". Isn't this sometimes a useful hint for users? | 10:38 |
| al-maisan | adeuring: not really .. it would rather confuse people | 10:38 |
| adeuring | al-maisan: OK, so r=me | 10:38 |
| al-maisan | adeuring: thanks | 10:39 |
| allenap | adeuring: Fancy a devscripts.ec2test branch to review? | 10:41 |
| allenap | https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-race-bug-422433/+merge/12778 | 10:41 |
| adeuring | allenap: sure | 10:41 |
| allenap | adeuring: Thanks :) | 10:41 |
| bigjools | al-maisan: don't remove that error text | 10:51 |
| al-maisan | bigjools: alright; what do you suggest instead? | 11:03 |
| bigjools | al-maisan: I don't have any context - what are you doing? | 11:03 |
| al-maisan | bigjools: https://bugs.edge.launchpad.net/soyuz/+bug/425800 | 11:04 |
| mup | Bug #425800: confusing message if signer can upload packages from some package sets but not others <packagesets> <soyuz-upload> <Soyuz:In Progress by al-maisan> <https://launchpad.net/bugs/425800> | 11:04 |
| bigjools | al-maisan: that error text is still correct, but only if the uploader has no packagesets | 11:05 |
| bigjools | he can upload to | 11:05 |
| al-maisan | bigjools: that's not quite right. | 11:06 |
| al-maisan | this error message indicates that the uploader has no upload permissions whatsoever | 11:06 |
| bigjools | al-maisan: yes, that's exactly what I mean | 11:06 |
| bigjools | so the existing check for that error text needs to also check packagesets | 11:07 |
| al-maisan | however, he may end up in this situation if he has *some* package set based permissions but is lacking one | 11:07 |
| bigjools | no, that is wrong | 11:07 |
| al-maisan | hmm .. I looked at the code .. the source package based permissions (including package sets) are checked before this error is raised | 11:08 |
| bigjools | the error should be raised if the upload has no component-based *and* no packageset-based permissions | 11:09 |
| al-maisan | is that error (formulated that way) useful in first place? | 11:09 |
| bigjools | yes - it distinguishes between NO permissions and SOME permissions but the wrong ones | 11:10 |
| al-maisan | aha | 11:10 |
| bigjools | PPA packages fall foul of it a lot, hence the extra text | 11:10 |
| * al-maisan takes another look at verify_upload() | 11:11 | |
| bigjools | al-maisan: basically if the uploader is trying to upload to say, main, but only has universe permissions, this error is not used | 11:12 |
| bigjools | the same needs to apply with packagesets | 11:12 |
| al-maisan | bigjools: I see what you mean. | 11:12 |
| bigjools | great | 11:12 |
| adeuring | allenap: r=me | 11:50 |
| allenap | adeuring: Thanks :) | 11:50 |
| adeuring | allenap: what I forgot to say iin the review: thanks for splitting the diff into two parts! | 11:51 |
| allenap | adeuring: I'm glad it helped :) | 11:52 |
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === matsubara-afk is now known as matsubara | ||
| === matsubara_ is now known as matsubara | ||
| allenap | adeuring: I replied to your review of ec2-test-race-bug-422433 with an incremental diff. Could you sanity check it, then I'll send it all off to ec2? Thanks. | 13:55 |
| === barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| adeuring | allenap: looks good. Thanks for the change! | 14:02 |
| allenap | adeuring: Thanks. | 14:02 |
| noodles775 | barry, have you checked some bug pages also? The "Bug #440220 reported by Barry Warsaw 16 hours ago" is also using the registration slot iirc... | 14:19 |
| mup | Bug #440220: registering slot interferes with watermark <post-ui-3-cleanup> <Launchpad Registry:In Progress by barry> <https://launchpad.net/bugs/440220> | 14:19 |
| noodles775 | which means that it will *always* wrap (unless you've got a 2m screen ;) ). | 14:19 |
| barry | noodles775: right. let me paste a bug page s/s | 14:20 |
| barry | noodles775: attached two more s/s. they do always wrap but it doesn't look too bad to me | 14:23 |
| noodles775 | barry, I think the thing I don't like is that it's wrapping when it doesn't need to. | 14:26 |
| noodles775 | Have you tried leaving it where it was after adding the no_wrap on the app-tabs? | 14:27 |
| barry | noodles775: it will run into the tabs if the registering contents are long (long person name). maybe that's rare enough to not worry about it though? | 14:28 |
| noodles775 | barry, if you give the registration slot a large left-margin (maybe 2em) it shouldn't get too close? (and will wrap since the app tabs can't, iirc?) | 14:30 |
| === Ursinha-afk is now known as Ursinha | ||
| barry | noodles775: e.g. this does not look as good to me: http://launchpadlibrarian.net/32862698/ff-nowrap.png | 14:30 |
| barry | noodles775: that's a possibility too. i kind of like that the registering information sits neatly above the portlet | 14:31 |
| noodles775 | barry, hmm.... to me it often (ie. with longer slots like on bugs pages) looks like it's been squashed into a space that's too small. | 14:33 |
| noodles775 | barry, I personally agree with rockstar's comment - if it was just below the app tabs, it wouldn't need to wrap at all, and would not interfere... but maybe there's a reason for not doing that? | 14:34 |
| barry | noodles775: it is a pretty small space (21%). | 14:34 |
| barry | noodles775: i definitely want to explore that, but iirc beuno really wants that registering slot on the right side | 14:34 |
| barry | noodles775: so i kind of wanted to run that to ground to see if we could make it work | 14:35 |
| noodles775 | barry, it would be on the right side wouldn't it? I just mean a little bit down from where it is in the last link above. | 14:35 |
| barry | noodles775: hmm... possibly so. give me a few minutes and i'll try that | 14:36 |
| noodles775 | k | 14:36 |
| barry | btw, thanks! | 14:36 |
| noodles775 | np! | 14:37 |
| barry | noodles775: take a look at the s/s in comments 13-16 | 14:48 |
| noodles775 | barry, what do you think? My first thought is that the only thing that looks ugly about it is the super long username? | 14:51 |
| barry | noodles775: agreed. i just did that to see worst case. this works for me | 14:53 |
| noodles775 | barry, great. Thanks for fixing it! | 14:55 |
| barry | noodles775: thanks! can i request you as one of the two ui reviewers for the branch? | 14:55 |
| noodles775 | barry, sure! | 14:55 |
| barry | awesome | 14:55 |
| barry | adeuring: ping | 15:07 |
| adeuring | barry: yes? | 15:07 |
| barry | adeuring: have time for a 39 line CSS review? | 15:07 |
| adeuring | barry: sure | 15:07 |
| barry | cool. sending mp now | 15:07 |
| adeuring | barry: I have also an MP (a bit larger than yours, but just trivial refactoring): https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 could you review it? | 15:20 |
| barry | adeuring: sure! this feels so incestuous though :) | 15:26 |
| adeuring | barry: ;) | 15:26 |
| adeuring | barry: r=me | 15:33 |
| === Ursinha is now known as Ursinha-lunch | ||
| barry | adeuring: thanks! | 16:06 |
| === barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| barry | adeuring: did you request me as a reviewer on your branch? | 16:11 |
| adeuring | barry: no | 16:11 |
| barry | adeuring: url? | 16:11 |
| adeuring | barry: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 | 16:11 |
| al-maisan | hello adeuring, can you please have a look at the fix for bug #425800 (2nd try) ? | 16:22 |
| mup | Bug #425800: confusing message if signer can upload packages from some package sets but not others <packagesets> <soyuz-upload> <Soyuz:In Progress by al-maisan> <https://launchpad.net/bugs/425800> | 16:22 |
| al-maisan | https://code.launchpad.net/~al-maisan/launchpad/cmsg-2-425800/+merge/12791 | 16:22 |
| adeuring | al-maisan: sure | 16:22 |
| al-maisan | adeuring: thanking you :) | 16:22 |
| barry | adeuring: when you raise NotImplementedError in your properties, you don't need to instantiate them if they have no arguments. Python will DTRT for you here, and it's also somewhat more efficient | 16:26 |
| adeuring | barry: Ah, thanks! didn't know that | 16:27 |
| adeuring | (Though I hope the excpetion will never be raised ;) | 16:27 |
| barry | adeuring: if the exception has arguments, instantiating it in place is the right thing to do though. | 16:28 |
| barry | adeuring: right! :) | 16:28 |
| adeuring | al-maisan: r=me | 16:32 |
| al-maisan | adeuring: thank you very much! | 16:33 |
| adeuring | al-maisan: immer gerne :) | 16:33 |
| al-maisan | adeuring: :) | 16:33 |
| barry | adeuring: with that fix, r=me | 16:38 |
| adeuring | barry: thanks! | 16:38 |
| === barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| barry | rockstar: ping | 16:40 |
| rockstar | barry, yo | 16:40 |
| barry | rockstar: hiya! could you do a quick second ui review on https://code.edge.launchpad.net/~barry/launchpad/440220-slot/+merge/12786 | 16:41 |
| rockstar | barry, sure. | 16:41 |
| barry | rockstar: thanks! | 16:41 |
| mrevell | barry: Can you take a look at small help branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-215798/+merge/12792 | 16:41 |
| barry | mrevell: sure thing | 16:42 |
| mrevell | thanks barry | 16:42 |
| barry | mrevell: did you forget to bzr add register-branch.html perhaps? it's not in the diff | 16:43 |
| * mrevell hangs head in shame | 16:43 | |
| mrevell | barry: Yup. Sorry. Pushing new version now | 16:44 |
| mrevell | barry: done | 16:44 |
| barry | mrevell: can you add the file as a comment. i don't think the mp will update the diff | 16:44 |
| rockstar | mrevell, I have bzr ci aliased to `bzr commit --strict` and I use that all the time. It makes sure you don't have any rogue files laying around. | 16:44 |
| barry | rockstar: +1 | 16:44 |
| rockstar | barry, it will update the diff. | 16:44 |
| mrevell | barry: I thought we had moving diffs now | 16:44 |
| mrevell | rockstar: thanks for the top! | 16:44 |
| rockstar | barry, it updates the diff on push of the source branch now. | 16:45 |
| barry | rockstar: \o/ | 16:45 |
| mrevell | s/top/tip | 16:45 |
| rockstar | barry, abentley is pretty amazazing. | 16:45 |
| barry | rockstar: he's amazing, too! | 16:45 |
| mrevell | :) | 16:46 |
| rockstar | barry, I was using a word from a movie, so the apparent typo was actually purposeful. | 16:46 |
| barry | rockstar: oh! what movie? | 16:46 |
| rockstar | barry, it's from "The Benchwarmers" The dialogue goes something like "You guys think you're ath-a-letes now?" "I didn't know athletes had three syllables. That's amazazing!" | 16:48 |
| barry | mrevell: r=me | 16:48 |
| mrevell | thanks barry | 16:48 |
| barry | rockstar: guess i have to put that on my netflix now :) | 16:48 |
| rockstar | barry, it's appropriate for Max too. | 16:49 |
| barry | rockstar: great review quotes: "filled with sprays of vomit, fountains of spit and enough hot body air to launch a flotilla of passenger balloons" and "a movie packed with so many idiot characters that rob schneider is cast as the cool guy -- and sort of pulls it off" | 16:52 |
| * barry clicks add to queue :) | 16:52 | |
| rockstar | barry, :) | 16:52 |
| === Ursinha-lunch is now known as Ursinha | ||
| === adeuring changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com | ||
| barry | EdwinGrubbs, rockstar ping | 21:58 |
| EdwinGrubbs | barry: pong | 21:58 |
| barry | hi EdwinGrubbs can you take a quick look at a testfix diff for me? | 21:59 |
| barry | EdwinGrubbs: http://pastebin.ubuntu.com/284096/ | 21:59 |
| EdwinGrubbs | barry: is the facetmenu class have any stylesheets, or is it just a marker? | 22:02 |
| barry | EdwinGrubbs: it has some css, but base-layout.txt doesn't test it. it is a horrible horrible test, so this just repairs it in the easiest possible way | 22:02 |
| EdwinGrubbs | barry: r=me | 22:05 |
| barry | EdwinGrubbs: thanks! testfix here i come | 22:06 |
| === rockstar` is now known as rockstar | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
