=== Ursinha_ is now known as Ursinha
=== 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
[08:49] <henninge> adeuring: Moin!
[08:49] <henninge> adeuring: I got onen for you if you llike ;-)
[08:50] <adeuring> henninge: sure
[08:51] <adeuring> henninge: this one: https://code.edge.launchpad.net/~henninge/launchpad/bug-568355-package-name/+merge/24844 ?
[08:52] <henninge> adeuring: yes, sorry. ;)
[08:52] <henninge> I was a still looking through it.
[08:52] <henninge> adeuring: that's the one
[08:52] <henninge> danke
[08:52]  * henninge needs to reboot
[09:11] <henninge> adeuring: any questions so far?
[09:11] <adeuring> EdwinGrubbs: not yet ;)
[09:12] <henninge> I will be relocating now, back in 20 min.
[09:12] <henninge> adeuring: and my name is henninge ;)
[09:12] <adeuring> henninge. EdwinGrubbs sorry
[09:21] <jtv> adeuring: mind finishing a small lazr.batchnavigator review?  https://code.edge.launchpad.net/~jtv/lazr.batchnavigator/bug-574159/+merge/24577
[09:22] <bryceh> would anyone be willing to review my branch at lp:~bryceharrington/launchpad/api-doc-fixes ?  This is my first launchpad merge proposal so apologies if I'm not doing something right
[09:22] <adeuring> jtv: sure; let me first finish a review for hennig
[09:22] <adeuring> bryceh: I can do it, after henning's and jtv's branches
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: henninge || queue: [jtv, bryceh] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[09:22] <jtv> adeuring: thx
[09:23] <bryceh> adeuring, ok thanks.
[10:01] <bryceh> adeuring, bedtime for me, I'll check back in the morning.
[10:11] <adeuring> henninge: lines 51-58 of the diff change the order of locations. The doc string of the method says "This function goes through the ordered list of these possible locations, the order having been copied from intltool-update". My question: Was the old order plain wrong or did the intltools chnage. IOW: Is the new ordering always correct?
[10:12] <adeuring> or can it be wrong for some versions of intltools?
[10:12] <henninge> adeuring: the ordering is correct, the comment is not valid any more.
[10:12] <adeuring> henninge: so, the old ordering was simply based on a misinterpretation of what intltools are doing?
[10:13] <henninge> adeuring: no, the original order was taken from intltool-update
[10:14] <henninge> adeuring: basically, what is says "use GETTEXT_PACKGE from Makefile.in.in or else from configure.ac or else from configure.in"
[10:14] <adeuring> henninge: ok... but the substitution order changed, right?
[10:14] <henninge> adeuring: this is now reflected in the "keep_trying" value.
[10:14] <adeuring> henninge: ah, I see. So, can you update the comment? otherwise, r=me
[10:15] <henninge> adeuring: yes, thanks
[10:15] <henninge> I am still trying to answer the question about the substitution ... ;)
[10:16] <henninge> give up )
[10:16] <henninge> :)
=== adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jtv || queue: [bryceh] || 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 || reviewing: bryceh || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[11:03] <adeuring> bryceh: I am not sure if we should use the term "patch attachment" as the title for the checkbox. The main bug page has two portlets "patches" and "bug attachments", thus distiguishing between patches and ohter attachments.
[11:03] <adeuring> Having the two words in the title for one search option might confuse users
[11:03] <adeuring> (on the "advanced search" page)
[11:24] <noodles775> Hi adeuring, if you can ignore the conflicts on the MP (and use the linked diff instead), would you be able to look at: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-3/+merge/24814
[11:24] <adeuring> noodles775: sure
[11:24] <noodles775> (for the same reason, to run the tests, you'll have to branch it rather than merge).
[11:24] <noodles775> Thanks!
=== noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: bryceh || queue: [noodles775] || 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 || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[11:24] <noodles775> :)
[12:16] <adeuring> noodles775: regarding the removeSecurityProxy() calls: I think the properties estimated_duration and status are simply missing in a set_attributes directive.
[12:27] <noodles775> adeuring: Yes, but I haven't changed the set_attributes (other than to change attribute names). That is, it was being set before without either the set_attributes or removeSecurityProxy.
[12:27] <noodles775> Unless I missed it somewhere else in the zcml.
[12:28] <adeuring> noodles775: that's odd.
[12:28] <noodles775> odd, or more likely something obvious that I missed in one of the 10 other branches in this pipeline :/
[12:28] <adeuring> noodles775: but whatever the reason is that it worked before: what about adding ZCML directives instead of using removeSecurityProxy?
[12:31] <noodles775> adeuring: but in most of those cases, I'm using it to setup a test situation which shouldn't normally be possible (like, for example, updating the distro_arch_series of a build, or the build's status).
[12:32] <noodles775> Or did you mean specifically for the buildqueue's estimated_duration? That would be ok.
[12:32] <adeuring> noodles775: right, I am not that much concerned about tests
[12:33] <adeuring> but I believe I spotted one other location, but can't find it again. (for estimated_duration)
[12:33]  * noodles775 looks to see where I've used rSP in the code... it's just for the temporary static methods (ie. that used to use self.attrname)
[12:34] <noodles775> adeuring: in buildbase.py, with the XXX and comment explaining why.
[12:38] <adeuring> noodles775: right, sorry, I needed some time to find it again locally ;)
[12:39] <adeuring> noodles775: otherwise, no complaints, so r=me
=== mrevell is now known as mrevell-lunch
[12:43] <noodles775> Thanks adeuring. I've got another one ready too: https://code.edge.launchpad.net/~michael.nelson/launchpad/ppa-generate-key-failure/+merge/24871
[12:43] <noodles775> wgrant: did you have any comment on the change I added for non-ascii chars^^
[12:43] <adeuring> noodles775: ok, but let me have lunch first ;)
[12:43] <noodles775> adeuring: of course... enjoy!
=== noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[13:18] <wgrant> noodles775: Ah, true, it was only reasonably safe things. But that looks much better (besides the lack of whoami on that revision).
[13:18] <wgrant> I don't think str() on a unicode is ever right.
[13:22] <noodles775> Yeah. (And yes, just setting up a new vm for dev work, I'll re-commit that rev).
[13:22] <noodles775> Thanks for the input!
[13:23] <wgrant> Thanks for fixing that.
[13:39] <adeuring> noodles775: I think it is a safe guess to use UTF8 encoding -- but shouldn't we add a test that filtering for a string conainting non-ascii character finds the expected data?
=== mrevell-lunch is now known as mrevell
[13:49] <noodles775> adeuring: as per my comment there, as far as I can see, this is only ever used for getting keys by email address or fingerprint (neither of which use non-ascii code), so I'm not sure how to test it (other than what I've done, ensuring it doesn't error). Any ideas?
[13:56] <adeuring> noodles775: we could add a test key having a "bad" name. But since you say that we seem to search only for email addresses and FPs, I think it is not that important. (sorry, missed that)
[13:57] <adeuring> noodles775: so, r=me
[14:01] <noodles775> adeuring: great, thanks.
=== 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
[14:53] <noodles775> wgrant: resending your branch now to ec2 now (let me know if you've already gotten someone else to do it.)
=== sinzui changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [sinzui] || 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 || reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
[15:46] <adeuring> sinzui: r=me; two nitpicks
[15:46] <sinzui> pick away
=== 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
=== salgado is now known as salgado-lunch
=== adeuring 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-lunch
=== salgado-lunch is now known as salgado
=== gary_poster is now known as gary-lunch
=== matsubara-lunch is now known as matsubara
[18:43] <salgado> I'm not really on call today, but I can do reviews if needed
=== gary-lunch is now known as gary_poster
[20:12] <abentley> rockstar, could you review my branch, please? lp:~abentley/launchpad/fix-encoded-attachments into	lp:launchpad/devel
[20:13] <abentley> https://code.launchpad.net/~abentley/launchpad/fix-encoded-attachments/+merge/24917
[20:22] <rockstar> abentley, looking
[20:24] <rockstar> abentley, r=me
[20:24] <abentley> rockstar, thanks.
[20:42] <mars> gary_poster, do you have a moment to review this config change?  It is very simple, just reorders the keys: https://code.edge.launchpad.net/~mars/launchpad/reorder_config_keys/+merge/24865
[20:44] <gary_poster> looking, mars
[20:51] <gary_poster> mars, mostly out of curiosity, do we have standards for this formatting, such as two CRs before each section start?
[20:51] <mars> gary_poster, nope, I think convention works well enough here.
[20:52] <mars> plenty of prior art.  violations stand out.
[20:52] <gary_poster> mars, couldn't the same be said of alphabetization?
[20:53] <mars> gary_poster, ?
[20:54] <mars> you mean, "It looks better without alphabetizing it, why should I"?
[20:55] <gary_poster> mars, IOW, you define an alphabetization standard in writing at the top of the doc, even though most things were alphabetized.  Convention did not seem to be sufficient.  Therefore, do we also need to document two CRs before each heading?  All that said, I think I'm being picky because I feel tired. ;-) I'll approve.
[20:56] <mars> \o/
[20:57] <mars> thanks Gary :)
[20:57] <gary_poster> :-)
[20:57] <mars> gary_poster, fwiw, that's why I personalized the message at the top.  Personalization can be a more powerful motivator than standards sometimes.
[20:58] <mars> It proves I care
[21:07] <mars> gary_poster, have a moment for one more, a trivial addition of an XXX comment?  https://code.edge.launchpad.net/~mars/launchpad/trivial-add-ec2-xxx/+merge/24927
[21:12] <gary_poster> alright mars
[21:16] <gary_poster> mars, approved with request to add date per the usual policy
[21:16] <mars> gary_poster, good catch, thanks
[21:16] <gary_poster> np
[21:17] <rockstar> Can I get a quick review from someone?  https://code.edge.launchpad.net/~rockstar/launchpad/recipe-polish/+merge/24928
[21:18] <rockstar> mars, ^^ ?
[21:18] <mars> rockstar, looking
[21:21] <mars> rockstar, why did you nuke all of that test output around line 49 of the diff?  I don't see anything in your code that would suggest its removal.
[21:22] <rockstar> mars, they were distroseries that have distributions that have no archives.
[21:22] <rockstar> mars, basically, this branch prevents someone trying to request a recipe build for lenny (Debian)
[21:23] <rockstar> ...because we don't support any distro but Ubuntu.
[21:23] <mars> oh!  So your fix works, because it proved that the tests themselves had invalid distros in there
[21:23] <rockstar> mars, yup.
[21:24] <mars> rockstar, ok.  Do you need a test for the explicit exclusion of a distro?
[21:24] <mars> rockstar, a unit test or something?
[21:24] <rockstar> mars, no, I don't think so.
[21:25] <rockstar> The test coverage is being accomplished properly with what we have now.
[21:25] <mars> ok
[21:27] <mars> rockstar, r=mars
[21:28] <rockstar> mars, thanks
=== salgado is now known as salgado-afk