| thumper | jml: https://code.launchpad.net/~thumper/launchpad/reassign-review-into-model/+merge/15991 if you can | 01:58 |
|---|---|---|
| thumper | jml: it would be good to get this landed today | 01:59 |
| jml | thumper, sure. I'll churn through a few action-demanding emails first. | 01:59 |
| thumper | jml: ok, ta | 01:59 |
| === rockstar changed the topic of #launchpad-reviews to: no one || reviewing: || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| thumper | jml: can I get you to do a very boring code review? | 04:21 |
| thumper | jml: it just moves stuff around in a template (mostly) | 04:21 |
| jml | thumper, yeah, sure. | 04:22 |
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/comment-ui-tweaks/+merge/15920 | 04:22 |
| thumper | I'm just looking into noodles's comment about enabling | 04:22 |
| thumper | I'm thinking he missed something | 04:22 |
| thumper | jml: if you want a picture I'll put one up for you to look at | 04:23 |
| thumper | jml: it is a big improvement | 04:23 |
| jml | thumper, that'd help, actually. | 04:24 |
| thumper | http://penhey.net/~tim/add-comment.png | 04:28 |
| thumper | jml: just afk for a few minutes | 04:28 |
| thumper | jml: back | 04:39 |
| jml | thumper, you haven't made any comments on that review recently | 04:39 |
| jml | thumper, max-width in #add-comment-form should have a space after the colon | 04:42 |
| thumper | ok | 04:42 |
| jml | I don't really like the text "Optional review" | 04:42 |
| thumper | jml: it was what our ui reviewer noodles suggested, and I think it is ok | 04:43 |
| thumper | jml: given the improvements this has in polish, I'd suggest that we land it and bikeshed about the text later | 04:44 |
| jml | thumper, what improvements does this have in polish? | 04:44 |
| jml | just moving the fields down below the text area? | 04:44 |
| thumper | jml: have you looked at the page recently? | 04:45 |
| thumper | look at the two in comparison and tell me it doesn't look more polished | 04:45 |
| jml | thumper, it looks better, sure. | 04:46 |
| jml | thumper, you don't mention the JS changes in the cover letter -- what are they for? | 04:48 |
| thumper | The comment block only used to be there when JS was enabled | 04:48 |
| thumper | I changed it to work without JS too | 04:48 |
| thumper | but it only shows if you are logged in | 04:48 |
| thumper | in the same way the bugs comment does | 04:48 |
| jml | ahh cool. | 04:49 |
| thumper | in fact the comment about being logged in came from the bugs page | 04:49 |
| thumper | consistency FTW | 04:49 |
| jml | great, the diff has changed since I started reviewing :( | 04:50 |
| thumper | jml: only two spaces added | 04:50 |
| jml | thumper, I've done a review. | 04:56 |
| thumper | ta | 04:56 |
| thumper | jml: actually the suggested text was "Optional review info:", do you think that is any better? | 05:02 |
| jml | thumper, I think it's more meaningful, yes. I think "info" is a bit too informal, but I'd be willing to let it pass. | 05:03 |
| jml | thumper, Why do you think the "optional" bit helps? | 05:03 |
| thumper | jml: can you think of something less formal? | 05:03 |
| jml | more formal :) | 05:03 |
| thumper | yeah, that | 05:04 |
| thumper | it is Friday afternoon | 05:04 |
| jml | thumper, "Optional review information" would be the obvious | 05:04 |
| thumper | I thought it was too informal to | 05:04 |
| thumper | that why I removed it | 05:04 |
| jml | yeah, I know. Fridays suck. | 05:04 |
| thumper | lemmie look at what that looks like | 05:04 |
| thumper | it looks very verbose | 05:05 |
| thumper | jml: in fact removing the Optional looks good to | 05:06 |
| thumper | just having Review there | 05:06 |
| thumper | so the default is: | 05:06 |
| jml | thumper, I think saying "Comment only" implies the optional, myself. | 05:06 |
| thumper | Review: Comment only | 05:06 |
| thumper | jml: so remove the Optional? | 05:06 |
| jml | thumper, yep. | 05:06 |
| thumper | ok | 05:06 |
| jml | thumper, I just replied to your other review | 05:07 |
| jml | aiui, if I try to assign a thing to you for review, and you are already assigned to do it | 05:07 |
| thumper | I'm not sure I'll get that finished tonight | 05:07 |
| jml | the error is "Tim Penhey already has an existing pending review" | 05:08 |
| jml | is that right? | 05:08 |
| thumper | almost: "Tim Penhey (thumper) already has an existing pending review" | 05:08 |
| jml | ahh ok. | 05:08 |
| jml | how about "Tim Penhey (thumper) has already been asked to review this" | 05:08 |
| thumper | sounds fine | 05:08 |
| thumper | better even | 05:09 |
| jml | and likewise, "Tim Penhey has already reviewed this" | 05:09 |
| jml | rather than 'has a personal review' | 05:09 |
| thumper | yeah, they sound good | 05:09 |
| jml | cool. | 05:09 |
| jml | makes it easier to understand what the code is for, also :) | 05:10 |
| === noodles775_ is now known as noodles775 | ||
| noodles775 | Hi Tim, yeah, the disabling/enabling of the review type field was nothing to do with your branch (it's been like that for a while) - I just wasn't sure if it was a decision, or a bug. | 08:20 |
| noodles775 | thumper: ^^ | 08:20 |
| thumper | noodles775: design decision and it is working as designed :) | 08:20 |
| thumper | noodles775: and now check out the revisions pushed interleaved with the comments \o/ | 08:21 |
| noodles775 | thumper: wow! | 08:21 |
| * noodles775 looks | 08:21 | |
| thumper | revisions hyperlinked and everything | 08:22 |
| * thumper does a little dance | 08:22 | |
| * thumper leaves | 08:24 | |
| noodles775 | Enjoy your w/e thumper! | 08:29 |
| noodles775 | Hi adeuring, could you take a quick look at bug 493703 for me? I'm wondering if it might be related to changes to the LFA/LFC last cycle. | 08:31 |
| mup | Bug #493703: LocationError raised in build page and distribution arch series binary package page <oops> <Soyuz:In Progress by michael.nelson> <https://launchpad.net/bugs/493703> | 08:31 |
| * noodles775 looks for the mp | 08:31 | |
| adeuring | npoosure | 08:31 |
| adeuring | noodles775: sure | 08:32 |
| noodles775 | ta | 08:32 |
| noodles775 | I don't suppose there are any ui reviewers able to do a second ui review for https://code.edge.launchpad.net/~michael.nelson/launchpad/211008-visual-indicator-superseded-pkgs/+merge/15963 | 08:48 |
| === danilo-bblot is now known as danilos | ||
| noodles775 | adeuring: I just added a comment on the bug with steps to reproduce locally. | 09:02 |
| noodles775 | (but still trying to find out why this might happen) | 09:02 |
| === 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 | ||
| gmb | adeuring: Could you take a look at https://code.edge.launchpad.net/~gmb/launchpad/taste-the-blood-of-ajax-dupefinding/+merge/16006 for me? | 10:06 |
| adeuring | gmb: sure | 10:06 |
| gmb | Ta | 10:06 |
| === jelmer changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: || queue [jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | adeuring: hi. can you please take a look at https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994 ? Thanks! | 10:24 |
| adiroiban | just add in your queue... there is no hurry | 10:24 |
| adeuring | adiroiban: sure, one I've finished gmb's and jelmer' MP. Just add yourself to the queue :) | 10:24 |
| === adiroiban changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: || queue [jelmer,adiroiban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | ok | 10:25 |
| adeuring | gmb: r=me | 10:45 |
| gmb | adeuring: Awesome, thanks! | 10:45 |
| jpds | adeuring: Can you land http://tinyurl.com/ydl256z for me? | 10:56 |
| adeuring | jpds: that MP needs a review first ;) But I'll add it to my queue. | 10:58 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: || queue [jelmer,adiroiban, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| danilos | adiroiban, hi, only a few small bits remaining for your permissions branch, if you've got them ready in the next few hours, I can probably land it for you later :) | 11:00 |
| jpds | adeuring: http://pastebin.ubuntu.com/339136/plain/ . :) | 11:00 |
| adiroiban | danilos: hi. yep. working on that... puzzled by the python-mode indentation :D | 11:01 |
| adiroiban | danilos: ok. got it. sorry | 11:02 |
| danilos | adiroiban, btw, it seems our automatic script to detect commits is not working properly, can you please update the bugs you know are fix committed to point to 3.1.12 release and tag them with qa-needstesting (or qa-ok if they are already on edge and you confirmed it's all fine :) | 11:02 |
| danilos | adiroiban, yeah, python-mode is not perfect, sometimes it just fucks it up, if you pardon my Romanian :P | 11:02 |
| adiroiban | danilos: ok. | 11:02 |
| adiroiban | i'll update them, no problem | 11:03 |
| danilos | cool, ta | 11:03 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: jelmer || queue [adiroiban, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === matsubara-afk is now known as matsubara | ||
| === salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jelmer || queue [adiroiban, jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| salgado | adiroiban, yours is https://code.edge.launchpad.net/~adiroiban/launchpad/bug-427319/+merge/15994 ? | 11:29 |
| adiroiban | salgado: yes. | 11:30 |
| adiroiban | danilos: the branch is almost ready. I have added the requried test for checking template admin task for product and distribution, including disabled templates... running the full test to check that everhing is ok | 11:32 |
| danilos | adiroiban, cool, if those tests pass, I'll be running the full test suite before landing anyway, so you don't have to | 11:32 |
| salgado | adiroiban, are you pushing more changes to this branch or are you talking about a different branch with danilos? | 11:34 |
| danilos | salgado, it's a different branch | 11:34 |
| salgado | ok, cool | 11:34 |
| gmb | on call: adeuring,salgado || reviewing: jelmer || queue [adiroiban, jpds, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | 11:39 |
| gmb | adeuring, salgado: Another one for you, when you've time https://code.launchpad.net/~gmb/launchpad/scars-of-ajax-dupefinding/+merge/16009 | 11:39 |
| salgado | gmb, sure, but only if you learn how to change the topic. ;) | 11:40 |
| gmb | salgado: AAAAARGH. | 11:40 |
| === gmb changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jelmer || queue [adiroiban, jpds, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| salgado | there you go. :) | 11:40 |
| gmb | salgado: Nearly 15 years I've been using IRC. Can I get it right? Can I hell. | 11:40 |
| adiroiban | danilos: done. changed pushed. diff added as a comment | 11:41 |
| === salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jelmer,adiroiban || queue [jpds, gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: jpds, adiroiban || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adeuring | noodles775, beuno: could you have a quick ui-look at this MP: https://code.edge.launchpad.net/~jpds/launchpad/fix_50580/+merge/15892 ? | 12:22 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: gmb, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | adeuring, jpds: done, thanks! | 12:32 |
| === henninge is now known as henninge-lunch | ||
| adeuring | noodles775: thanks! jpds: I'll run your branch through the EC2 tests | 12:33 |
| jpds | noodles775: Yeah, I have nothing to do with the existing test mirror data... | 12:34 |
| jpds | adeuring, noodles775: Thanks for the reviews. :) | 12:34 |
| noodles775 | jpds: np. Yes, I didn't mean updating the test/sample data, just either some steps to do, or a small python script that the reviewer can run to add some interesting data to see the results. | 12:35 |
| jpds | noodles775: I'll look into it. | 12:36 |
| noodles775 | Just for next time... or a screenshot can sometimes be just as useful. | 12:36 |
| adeuring | gmb: r=me | 12:45 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, adiroiban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | adeuring: Fantastisch! Danke! | 12:46 |
| adeuring | gmb: welcome :) | 12:46 |
| === mrevell is now known as mrevell-lunch | ||
| === henninge-lunch is now known as henninge | ||
| jelmer | adeuring, thanks for the reviews :-) | 12:59 |
| adeuring | jelmer: welcome :) | 13:00 |
| === noodles775 changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, adiroiban || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | adeuring: could you pls fit this one in sometime? https://code.edge.launchpad.net/~michael.nelson/launchpad/493703-location-error/+merge/16015 | 13:06 |
| adeuring | noodles775: sure | 13:06 |
| noodles775 | Ta! | 13:07 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: noodles775, adiroiban || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adeuring | noodles775: r=me | 13:24 |
| === salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: noodles775 || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| mars | salgado, ping, have time to review a one-line patch? http://pastebin.ubuntu.com/339212/ | 13:30 |
| salgado | sure | 13:30 |
| salgado | mars, r=me | 13:31 |
| mars | thanks salgado | 13:31 |
| adiroiban | salgado: thanks for the review. I'll rework it. | 13:34 |
| salgado | adiroiban, thank you for the nice branch! | 13:35 |
| adiroiban | salgado: for the baseclass, can I use the abstract keyword, to prevent direct instantiation ? | 13:36 |
| salgado | adiroiban, in general we prefix the class name with Base, but I don't have a problem with abstract | 13:37 |
| adiroiban | salgado: ah. OK. I will go for Base then | 13:38 |
| salgado | adeuring, the queue's currently empty or is there a branch from noodles775 that has to be reviewed? | 13:40 |
| adeuring | salgado: my fault, i reviewed it and forgot to removed it from the topic... | 13:40 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| salgado | adeuring, it's still in the queue. ;) | 13:42 |
| adeuring | argh... | 13:43 |
| === adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === mrevell-lunch is now known as mrevell | ||
| adiroiban | salgado: is there an equivalent call in python for getting this string from TAL: view/translation_team/translator/fmt:link ? | 13:51 |
| salgado | oh, yeah, I forgot to mention that | 13:51 |
| salgado | just a sec | 13:51 |
| adiroiban | just a hint | 13:51 |
| salgado | these are formatters and they come from lib/canonical/launchpad/webapp/tales.py | 13:51 |
| adiroiban | and I will look into the code. | 13:52 |
| adiroiban | ok. so I should just call the formater | 13:52 |
| salgado | PersonFormatterAPI(naked_team).link(None) | 13:52 |
| salgado | yep | 13:52 |
| adiroiban | ok. I was not sure if this is the right way to do it... I was trying to keep those things in the template | 13:53 |
| adiroiban | and have only logic in the view | 13:53 |
| salgado | in this case we're just using the formatter, so we don't need to have html code in the middle of our python code. | 13:54 |
| salgado | we try hard not to have html together with python code, but in some cases it's the least bad option | 13:55 |
| adiroiban | ok. thanks | 13:55 |
| deryck | adeuring or salgado -- I've got an easy Windmill fix branch. | 14:16 |
| === deryck changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [deryck] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adeuring | deryck: I'll look at it | 14:16 |
| deryck | adeuring, thanks! | 14:17 |
| deryck | adeuring, https://code.launchpad.net/~deryck/launchpad/windmill-also-affects-link-failure-494018/+merge/16019 | 14:17 |
| adeuring | deryck: r=me | 14:21 |
| deryck | adeuring, excellent, thanks. I have another easy one if you like. :) | 14:22 |
| adeuring | deryck: sure | 14:23 |
| deryck | adeuring, waiting on the mp to appear | 14:23 |
| deryck | adeuring, https://code.edge.launchpad.net/~deryck/launchpad/remove-double-heading-tags-page/+merge/16020 | 14:26 |
| bac | g'morning sinzui | 14:31 |
| sinzui | hi bac | 14:31 |
| bac | sinzui: looking at the data from the query you ran yesterday made me realized i'd missed a case wrt bzip2 | 14:32 |
| adeuring | deryck: r=me, again | 14:32 |
| bac | sinzui: could i get a quick review of http://pastebin.ubuntu.com/339242/ | 14:32 |
| deryck | adeuring, thanks | 14:32 |
| === deryck changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| sinzui | bac: r=me | 14:33 |
| bac | sinzui: thanks. /me sends to pqm and back to bed... | 14:33 |
| === salgado is now known as salgado-lunch | ||
| gary_poster | BjornT_: hey. So, my only concerns are these. (1) should we be more careful to only do what we expect to be doing? IOW, I'd feel better if we had | 15:04 |
| gary_poster | if not safe_hasattr(sys.stdin, 'fileno'): | 15:04 |
| gary_poster | assert isinstance(sys.stdin, zope.testing.testrunner.runner.FakeInputContinueGenerator), 'Unexpected value for sys.stdin' | 15:04 |
| gary_poster | (2) should we try getting this upstream? It seems reasonable | 15:04 |
| gary_poster | BjornT_: otherwise +1 | 15:05 |
| gary_poster | BjornT_: I have someone waiting on a call. I'll mark this Approved: merge-conditional with those comments so you can proceed. If you disagree with my points, ping me and I'll reply asap | 15:06 |
| BjornT_ | gary_poster: i agree with your points. 2) was the reason i asked you for a review, since i thought you might now a thing or two about zope.testing :) | 15:08 |
| BjornT_ | i'm adding the assert, it's good to have | 15:08 |
| gary_poster | BjornT_: cool :-) | 15:09 |
| gary_poster | approved in the MP | 15:09 |
| BjornT_ | thanks! | 15:09 |
| === matsubara is now known as matsubara-lunch | ||
| === noodles775 changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === beuno is now known as beuno-lunch | ||
| noodles775 | adeuring (or salgado): Same bug, different call-site (when you've time): | 15:34 |
| noodles775 | https://code.edge.launchpad.net/~michael.nelson/launchpad/493703-bpr-error/+merge/16026 | 15:34 |
| adeuring | noodles775: sure, I'll look | 15:34 |
| noodles775 | Thanks again. | 15:35 |
| noodles775 | adeuring: in retrospect, I think the approach here would have been better for the build page too, but I won't get to change it before finishing today. | 15:35 |
| adeuring | noodles775: well, I think your previous branch was strictly bad ;) | 15:36 |
| adeuring | so, no problem if it stays for one cycle, I think | 15:36 |
| noodles775 | huh? | 15:37 |
| adeuring | noodles775: i mean: no problem with your branch from my side | 15:37 |
| adeuring | ...with the provious branch... | 15:37 |
| noodles775 | ah, I was wondering why you'd approved it if you thought it was strictly bad ;) | 15:37 |
| adeuring | noodles775: argh... now i see... i omitted a "not"... sorry... | 15:38 |
| noodles775 | :) | 15:38 |
| allenap | adeuring: Up for a quick review? https://code.edge.launchpad.net/~allenap/launchpad/is-user-affected-performance/+merge/16027 | 15:41 |
| adeuring | allenap: sure | 15:41 |
| adeuring | noodles775: r=me | 15:43 |
| noodles775 | Thanks adeuring | 15:43 |
| adeuring | allenap: nice change! r=me | 15:51 |
| allenap | adeuring: Thanks :) | 15:52 |
| === henninge is now known as henninge-brb | ||
| === salgado-lunch is now known as salgado | ||
| allenap | adeuring: I noticed another performance thing that I'd like to land with the patch you just reviewed. Mind reviewing it too? http://pastebin.ubuntu.com/339296/ | 16:06 |
| adeuring | allenap: sure | 16:06 |
| adiroiban | salgado: there is one thing, where I don't know what I should change http://paste.ubuntu.com/339303/ | 16:07 |
| salgado | adeuring, just change the narrative to make it clear you're creating that stuff just so that you can test what the page looks like when X and Z exist | 16:09 |
| adeuring | salgado: I believe you menat adiroiban ;) | 16:09 |
| salgado | yeah, I did | 16:10 |
| salgado | adiroiban, ^ | 16:10 |
| adeuring | allenap: very nice! r=me | 16:10 |
| allenap | adeuring: Thanks again :) | 16:10 |
| === henninge-brb is now known as henninge | ||
| adiroiban | salgado: like adding : This is requried for setting up the testing environment. | 16:12 |
| salgado | yeah, or just "Create X and Z so that we can see what the page will look like when they exist." | 16:12 |
| adiroiban | I'm quite a bad narrator | 16:12 |
| adiroiban | thanks | 16:13 |
| salgado | adiroiban, the narrative in your tests is quite good, actually; just this bit that needed some tweaks | 16:13 |
| adiroiban | salgado: ok. I have pushed the changes and added a diff with the latest changes | 16:18 |
| allenap | adeuring: Interesting. Looks like there's a new feature in merge proposals. It shows the change I added after your review in https://code.edge.launchpad.net/~allenap/launchpad/is-user-affected-performance/+merge/16027 | 16:24 |
| adeuring | allenap: right! I saw this berfore today but did not look that closely | 16:26 |
| === matsubara-lunch is now known as matsubara | ||
| salgado | adiroiban, cool, I'll get to it in a minute | 16:28 |
| adiroiban | salgado: ok. no hurry. I just wanted to know if I followed the right process | 16:30 |
| salgado | adiroiban, ideally you should reply to the review, commenting on each of the points brought up by the reviewer; be it to agree, disagree, ask questions, propose a different alternative, etc | 16:33 |
| salgado | that's a good idea as it works like a check list of the things that need to be done | 16:34 |
| adiroiban | aha... so the review chat should be done by mail...as much as possible ? | 16:35 |
| adiroiban | and I don't need to ping the review to look at the changes | 16:36 |
| adiroiban | or land the branch afther it was approved | 16:36 |
| salgado | adiroiban, yes, I (and probably all reviewers) do it all by email, but if you prefer you can use the web UI. in general you don't need to worry about pinging them when you reply to the review or something, unless they take too long to get back to you | 16:39 |
| adiroiban | how many days are too long ? :) | 16:41 |
| salgado | adiroiban, when the reviewer is on call, 0.1 days is too long for me. ;) | 16:42 |
| adiroiban | ok. so if I will no solve the problem in the same day, I will have to wait for next week? | 16:43 |
| === beuno-lunch is now known as beuno | ||
| salgado | adiroiban, no, your reviewer should be happy to do a second round of review (or just ack your changes) when they're not on call. it might take a bit longer but never more than 1 or 2 days | 16:45 |
| salgado | and if your original reviewer is too busy they'll probably say so and you can ask another reviewer to continue from where the previous one left | 16:45 |
| adiroiban | many thanks, | 16:45 |
| adiroiban | so basically, to start a review, I only have to add myself in the IRC topic, and then communicate via email | 16:46 |
| salgado | adiroiban, yup. once your name is in the queue we'll look up your branch on https://code.edge.launchpad.net/launchpad/+activereviews or ping you if we can't find it | 16:48 |
| adiroiban | great | 16:52 |
| adiroiban | or I can just add adiroiban(bug-32323) ? | 16:53 |
| adiroiban | or MP id? | 16:54 |
| salgado | any of them would work | 16:59 |
| salgado | adiroiban, can you get a couple screenshots of that translations page after your changes for a UI review? | 17:34 |
| adiroiban | salgado: sure. It is ok if I attach them to the bugreport? | 17:35 |
| salgado | adiroiban, sure! then you can ask beuno or noodles775 for a UI review. :) | 17:35 |
| === deryck is now known as deryck[lunch] | ||
| beuno | I can review | 17:36 |
| adiroiban | beuno: I'm taking the screenshots | 17:36 |
| beuno | cool | 17:37 |
| salgado | noodles775, has your branch been reviewed already? | 17:47 |
| adiroiban | beuno: screenshots here https://bugs.edge.launchpad.net/rosetta/+bug/427319 | 17:49 |
| * beuno looks | 17:49 | |
| beuno | adiroiban, nice improvement | 17:50 |
| beuno | a few questions | 17:51 |
| beuno | why not "Your suggestions will be held for review by $TEAM"? | 17:51 |
| beuno | there's also a missing full stop | 17:53 |
| noodles775 | salgado: yep. | 17:53 |
| beuno | after the team's name | 17:53 |
| === noodles775 changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| beuno | before "Templates which are..." | 17:53 |
| beuno | other than that, ui=me | 17:53 |
| adiroiban | beuno: ok. I will change that. | 17:54 |
| === EdwinGrubbs is now known as Edwin-lunch | ||
| jpds | adeuring: EC2 test go OK? | 17:56 |
| adiroiban | how should I wrap this line: translations_contact_name = self.translation_team.translator.displayname | 18:37 |
| salgado | adiroiban, I'd use parenthesis | 18:37 |
| salgado | translations_contact_name = ( | 18:38 |
| salgado | self.translation_team.translator.displayname) | 18:38 |
| salgado | I think that's the preferred way in Launchpad | 18:38 |
| adiroiban | salgado: many thanks. I just wanted to be sure. | 18:38 |
| === adeuring changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === deryck[lunch] is now known as deryck | ||
| adiroiban | salgado: should I append the new diff to the reply email ? | 19:44 |
| salgado | adiroiban, if you think it'd be worth me having a look, sure | 19:45 |
| EdwinGrubbs | salgado: hey, I have a 1128 line diff, but most of it is generic changes like indenting a function converted to a method and substsitutions like s/client/self.client/. Since I am just moving the registry windmill tests into the lp/registry/windmill directory, I could easily split up this diff, but that would only be benefiicial if somebody else would review the other half. | 19:57 |
| salgado | EdwinGrubbs, I'll be leaving in less than 10 minutes, so I won't be able to review it, sorry | 19:58 |
| * EdwinGrubbs cries | 19:58 | |
| EdwinGrubbs | sinzui: would you be able to review a boring 1128 line diff for the windmill tests? | 20:00 |
| sinzui | yes, in about 20 minutes | 20:01 |
| === salgado 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 | ||
| salgado | adiroiban, just replied. I'll step out for about an hour, but I'll be back later to submit your branch to ec2 | 20:17 |
| adiroiban | salgado: ok. I'm also out | 20:17 |
| === salgado is now known as salgado-afk | ||
| EdwinGrubbs | sinzui: here is the mp: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-495067-move-windmill-tests/+merge/16057 | 20:23 |
| sinzui | EdwinGrubbs_: r=me, land it if you can | 22:06 |
| EdwinGrubbs_ | sinzui: thanks | 22:07 |
| === EdwinGrubbs_ is now known as EdwinGrubbs | ||
| === fjlacoste is now known as flacoste | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
