/srv/irclogs.ubuntu.com/2009/10/30/#launchpad-reviews.txt

rockstarmwhudson, around you around to take a branch?04:30
rockstarEr, take==review04:30
mwhudsonrockstar: hmm, maybe04:30
mwhudsonrockstar: how big?04:31
rockstarmwhudson, it's not very big.  One new method on IBranch.04:31
mwhudsonrockstar: ok then04:31
rockstarmwhudson, here's the diff: http://pastebin.ubuntu.com/304776/04:36
rockstarmwhudson, and here's the mp: https://code.edge.launchpad.net/~rockstar/launchpad/ibranch-upgrade/+merge/1420104:36
mwhudsonrockstar: well that was easy04:40
rockstarmwhudson, I aim to please.04:41
rockstarmwhudson, if I change that assert, I can probably remove the other assert as well, ent?04:42
mwhudsonrockstar: actually, the assert probably won't work as is04:45
mwhudsonrockstar: i think you probably want assertEqual([job], list(getUtility(IBranchUpgradeJobSource).iterReady()))04:45
mwhudsonrockstar: if you can get something like that working then yes, you don't need the other assert04:45
=== Ursinha is now known as Ursinha-afk
wgrantnoodles775, al-maisan: Can I grab one of you to look at https://code.edge.launchpad.net/~wgrant/launchpad/bpr-ddeb_package-db/+merge/14008?09:21
al-maisanhello wgrant, is it something that needs to land today?09:22
al-maisanIt's Friday of week 3 :P09:22
noodles775wgrant: debug_package +109:22
wgrantnoodles775: OK. I was following bigjools' advice with ddeb_package.09:23
noodles775wgrant: although, I'm guessing Julian would agree with your original name...09:23
noodles775wgrant: yes, exactly09:23
wgrantal-maisan: Hm, forgot it was that late, sorry.09:23
al-maisanwgrant: no problem .. it is just that I really need to land 2 branches myself today.09:24
noodles775wgrant: so although i'd prefer debug_package, I think we can assume that julian would go for ddeb_package, but if it doesn't need to land now, he'll be around next week to confirm.09:24
wgrantnoodles775: It doesn't need to land this cycle, as the rest won't.09:24
wgrantSo I guess I shall confirm with him.09:25
wgrantSorry for the noise.09:25
al-maisanhello adeuring, I am in dire need of a review (https://code.edge.launchpad.net/~al-maisan/launchpad/newuris-399186/+merge/14204)09:32
al-maisanwould you be willing to look at the branch above?09:32
adeuringal-maisan: review just done :) r=me09:32
al-maisanadeuring: thanks .. but this another branch :P https://code.edge.launchpad.net/~al-maisan/launchpad/newuris-399186/+merge/1420409:33
adeuringal-maisan: Ah, i'll look at it09:33
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: al-maisan || queue: [leonardr, maxb, salgado] || This channel is logged: http://irclogs.ubuntu.com/
* al-maisan sends virual cookies to adeuring 09:34
al-maisan*virtual09:34
adeuringal-maisan: could you add a doc string for the new method traverse() (diff line 21)09:39
al-maisanadeuring: will do.09:40
adeuringal-maisan: in ArchivePermissionSet._nameToPackageSet() you are using ubuntu.currentseries as the series. Is there no use case where this method should search for a package in another series?09:52
adeuring...i mean search for a pkg set09:52
al-maisanadeuring: the "problem" here is that the user only specified a package set *name* which is not enough to find a package set09:53
adeuringal-maisan: right. 09:53
al-maisanin other cases where the user does not specify the distro series for the package set we use `currentseries` as a default09:53
al-maisanusing it here is hence consistent with the rest of the package set API09:54
al-maisanif they want a package set in a different series they can pass in a `IPackageset` instance as opposed to a name09:55
adeuringal-maisan: Ah, now I undertsnd! thanks09:55
al-maisanin which case there is no need to second guess their intentions :)09:55
al-maisanyup09:55
BjornT_adeuring: could you review https://code.edge.launchpad.net/~bjornt/launchpad/windmill-more-problems/+merge/14172? i didn't add it to the irc topic yesterday, since i wouldn't be around for the review. but if i would have added it, i would have been first in the queue :)10:00
adeuringBjornT_: sure10:00
BjornT_thanks. it should be quite quick, it's only changes to tests10:01
adeuringal-maisan: s/aleady/already/ in the added tests.10:05
adeuringal-maisan: r=me10:05
al-maisanadeuring: thanks! Will fix that.10:05
al-maisanthat = typo10:06
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: BjornT_ || queue: [leonardr, maxb, salgado] || This channel is logged: http://irclogs.ubuntu.com/
=== adeuring changed the topic of #launchpad-reviews to: adeuring || reviewing: salgado || queue: [leonardr, maxb] || This channel is logged: http://irclogs.ubuntu.com/
daniloshenninge, jtv: anyone of you have time to take a look at my branch for #459132 (350 lines)?11:17
mupBug #459132: Clean up existing untranslated credits messages <Launchpad Translations:In Progress by danilo> <https://launchpad.net/bugs/459132>11:17
jtvdanilos: I don't, sorry!11:17
danilosjtv, ok, that's a shame :) let's see if henning does or will I have to go for a slot in the queue :)11:18
henningedanilos: will look,  if you look at mine ;-)11:18
jtvdanilos: queue's pretty full, so I'd take henning's offer :)11:18
daniloshenninge, sounds good, diff line for line :)11:18
danilosjtv, it's not like I haven't noticed :)11:19
jtvthat's why you asked in the first place... got it.11:19
henningedanilos: although I am liable to drop out at any time ... ;-)11:21
henningedanilos: here is my mp, https://code.edge.launchpad.net/~henninge/launchpad/bug-461756/+merge/1421111:21
daniloshenninge, mine is at https://code.edge.launchpad.net/~danilo/launchpad/bug-459132/+merge/1421011:21
henningeI was about to ask ...11:21
daniloshenninge, just one question about yours: is there a reason not to default to ordered_by_names=True?11:24
henningedanilos: the risk of breaking other code/tests. That is the only reason.11:25
henningebackward compatibility11:25
daniloshenninge, right, it'd be nice to see how much stuff breaks, but not that important right now11:25
daniloshenninge, it looks good, a very nice branch, r=danilo :)11:26
henningedanilos: thank you11:26
daniloshenninge, judging by the complexity of our branches here, I'd say you didn't get the better of it :)11:28
henningedanilos: I wasn't expecting that ... :-/11:29
daniloshenninge, it's not too bad I hope :)11:33
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [leonardr, maxb] || This channel is logged: http://irclogs.ubuntu.com/
adeuringleonardr: I see that you are in the review queue but i can't find any MP from you11:36
leonardradeuring: i'll look it up11:36
leonardryou probably dont' see it because it's a launchpadlib branch11:36
leonardrhttps://code.edge.launchpad.net/~leonardr/launchpadlib/trusted-workflow-tests11:36
leonardrer, https://code.edge.launchpad.net/~leonardr/launchpadlib/trusted-workflow-tests/+merge/1416711:37
=== jtv is now known as jtv-afk
adeuringtopic on call: adeuring || reviewing: leonardr || queue: [maxb] || This channel is logged: http://irclogs.ubuntu.com/11:38
henningedanilos: no, it's fine. After all, I am familiar with the stuff.11:39
henningedanilos: I see you use the storm "In" operator in pofile.py. I had tried to use it in potemplate.py which your branch touches, too.11:40
henningedanilos: it didn't work some how. You left the sql code in there (line 85), did you try to use storm In here, too?11:41
daniloshenninge, I haven't tried, most likely11:41
daniloshenninge, I can try it and it should probably work11:42
henningedanilos: would be nice. I cannot remember why I couldn't get it to work.11:43
daniloshenninge, right, I'm trying it out11:44
daniloshenninge, doesn't work for me either: "NameError: global name 'In' is not defined" :) ok, ok, I'll add an import as well :)11:44
daniloshenninge, doc/potemplate.txt passes if I use In() there11:45
daniloshenninge, http://paste.ubuntu.com/304971/11:46
henningedanilos: also plase fix the docstrig for CreditsFixer which was copied from the statistics script.11:47
henningedanilos: very nice, thank you11:48
daniloshenninge, how could you tell? :)11:48
henninge:)11:49
adeuringleonardr: you are raising RequestTokenAlreadyAuthorized(self.message(self.NONEXISTENT_REQUEST_TOKEN) -- the combintaion of parameter name/content and the exception name looks a bit odd ;) What about using TokenAuthorizationException?11:52
leonardradeuring: ok. the most likely reason the token woudln't exist is that it had already been authorized and exchanged11:53
leonardri'm actually thinking of using TokenAuthorizationException for everything11:53
leonardri don't know if i need all those exception types11:53
adeuringleonardr: OK, could you either add a comment to the existing variant or switch to TokenAuthorizationException ?11:55
leonardri will either switch to TAE or create a new kind of exception11:55
henningedanilos: your tests in test_potmsgset.py don't need to call setTranslationCreditsToTranslated explicitely because it is called implicitely now when the POTMsgSet is created.11:55
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue: [leonardr, maxb] || This channel is logged: http://irclogs.ubuntu.com/
salgadoadeuring, are you reviewing any of the branches from the queue?11:59
daniloshenninge, right, which means that the test is a bit flakey; how would I test that with the existing data, when there already is a translator-credits message with diverged, imported, current translation, it works correctly?11:59
adeuringsalgado: yes; acutally, I'm reviewing leonard's branch, and reviewed maxb's branch already...12:00
daniloshenninge, i.e. how do I work around that feature of the POTMsgSet creation to be able to test with data as it exists today in LP12:00
adeuringon call: adeuring,salgado || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ 12:00
henningedanilos: yeah, it would require a flag all the way down to where the POTMsgSet is created to *not* add the dummy translation.12:01
adeuringleonardr: a suggestion: you are showing several messages with a simple "print message". Would it make sense to mode the print statement into its own method showMessage() so that people who write GUI client have a single method they need to oberload?12:01
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: leonardr,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
adeurings/mode/move/12:02
henningedanilos: you could add that flag and remove it later once the script has run.12:02
henningedanilos: is there no relvant case in the test data?12:02
leonardradeuring: i do that in the next branch12:02
maxbadeuring: thanks - will you take care of PQMing it for me?12:02
adeuringleonardr: ok, cool12:02
daniloshenninge, there might be, but I hate using sampledata for tests :)12:02
adeuringmaxb: sure, will do that. 12:03
henningedanilos: then we need the flag, which isn't pretty either since it is only transitional.12:03
daniloshenninge, though, in this particular case, that sounds better than a flag12:03
henningedanilos: I'd say, if there is the right data. Creating new sample data would be dumb, too.12:04
daniloshenninge, of course12:05
henningedanilos: if you'll excuse me, I have a stand-up call now ... ;-)12:05
salgadoadeuring, can you run 'make check_db_merge' on a pristine devel branch and tell me if it fails?12:06
daniloshenninge, there are 3 translation credits messages in LP, that should do it12:06
daniloshenninge, heh, right :)12:06
adeuringsalgado: just a second...12:06
adeuringleonardr: r=me12:06
adeuringon call: adeuring,salgado || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/12:07
salgadoadeuring, that's not updating the topic. ;)12:07
adeuringarghhh12:08
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
adeuringnet my best day....12:08
salgadoadeuring, did you get around to running that check_db_merge thing?12:14
adeuringsalgado: just ran it: no problems12:15
adeuring(had to make coffe first...)12:15
salgadoit fails for me on a mainline branch12:15
salgadoI must have a conflict in a sourcecode/ branch or something12:16
intellectronicaadeuring: can you review a patch for me? it's 584, but all removals :)12:19
intellectronicaadeuring: it's the patch for killing the top-level +filebug12:19
adeuringintellectronica: I'd like to start some work on a small branch myself, so I'd prefer it if you could ask salgado too ...12:20
salgadosure!  I love removals12:20
intellectronicaadeuring: np12:20
intellectronicasalgado: great! http://pastebin.ubuntu.com/304994/12:20
salgadointellectronica, why is it being removed?12:21
intellectronicasalgado: for a long time we thought that it's not really useful, but after changing the workflow for filing ubuntu bugs we needed to do something about filing them from the top-level. we decided that we're just going to kill it12:22
salgadointellectronica, ok.  and how about the tests removed from 'lib/lp/bugs/stories/guided-filebug/xx-filing-security-bugs.txt'?  are aren't we losing some coverage by removing that?12:25
intellectronicasalgado: no, these tests only cover filing security bugs from the top-level (the test is inappropriately named). filing security bugs from pillar contexts is well covered elsewhere12:26
salgadointellectronica, cool. r=me then12:27
intellectronicasalgado: thanks!12:27
=== mrevell is now known as mrevell-lunch
=== sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
=== mrevell-lunch is now known as mrevell
henningedanilos: review sent13:27
daniloshenninge, thanks13:27
sinzuibarry:  stand-up in 2 minutes13:30
=== noodles775 changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [sinzui, noodles] || This channel is logged: http://irclogs.ubuntu.com/
noodles775adeuring or salgado: could one of you look at this trivial branch: https://code.edge.launchpad.net/~michael.nelson/launchpad/458200-p3a/+merge/1421614:33
noodles775it's mostly tests.14:33
adeuringnoodles775: I'm just writing am MP myself; when that's done, I'll look at it.14:34
noodles775Thanks!14:34
salgado+ self.libraryfile.expires = datetime.utcnow().replace(tzinfo=pytz.utc)15:01
salgadoadeuring, why do you do that instead of "datetime.now(pytz.UTC)"?15:01
adeuringsalgado: this gives an error when the value is dtpred in the DB. I found the replace() call in several places; simply copied it15:03
adeurings/dtpred/stored/15:03
salgadohmm15:04
salgadothat's weird15:05
adeuringsalgado: seems that I was a bit confused... I'll use datetime.now(pytz.utc)15:05
salgado+ >>> libraryfile.expires <= datetime.utcnow().replace(tzinfo=pytz.utc)15:06
salgadoadeuring, you can use that in the test too15:06
adeuringsalgado: right15:06
salgadoand I think pylint is on crack, so there's no need to ignore the warning there15:07
adeuringsalgado: perhaps; so you think I should removed the suppression again, so that we know that we should send pylint to a drug therapy ;)?15:08
salgadoadeuring, that's what I do in these cases, but it might be time for heavier drugs as I've been seeing this for quite some time now15:09
adeuringsalgado: ;) OK, removed f040115:09
=== salgado is now known as salgado-lunch
deryckadeuring or salgado, can I get in queue for a simple lint clean up branch?15:13
adeuringderyck: sure15:13
=== deryck changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [sinzui, noodles, deryck] || This channel is logged: http://irclogs.ubuntu.com/
adeuringlet me just finish another review15:14
deryckok, thanks, adeuring 15:14
sinzuisalgado-lunch: adeuring I have written a replacement lint for gedit-developer-plugins (launchpad uses a version of my lint script from 2002). I think I can adapt my 100% script to launchpad. It uses PEP8.py instead of pylint.15:15
adeuringsinzui: nice idea!15:16
sinzuiadeuring: I meant to say my script is 100% python. it checks python syntax + style, markup (knows about HTML entities), doctests, and plain-text files.15:18
adeuringsinzui: sounds really good!15:19
sinzuiIt is better than good, it is fast15:19
adeuringnoodles775: r=me15:20
noodles775Thanks adeuring 15:22
adeuringwelcome :)15:22
adeuringderyck: r=me15:28
deryckadeuring, thanks!15:28
sinzuiadeuring: can you review my blueprint branch?15:29
adeuringsinzui: sure15:29
sinzuiadeuring: https://code.edge.launchpad.net/~sinzui/launchpad/sprint-attendence-115:29
=== adeuring changed the topic of #launchpad-reviews to: adeuring,salgado || reviewing: sinzui,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
sinzui^ I had to write a lot of tests before I could add the new feature because there was nothing veriying the current behaviour.15:30
adeuringsinzui: r=me16:11
sinzuithank16:11
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
=== salgado-lunch is now known as salgado
sinzuiadeuring: your grammar is correct. My fingers do not like to type suffixes. I type a lot of unconjugated words.16:20
adeuringsinzui: ;)16:20
=== deryck is now known as deryck[lunch]
=== sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
=== beuno is now known as beuno-lunch
leonardradeuring or salgado, can one of you review https://code.edge.launchpad.net/~leonardr/launchpadlib/trusted-workflow-tests-2/+merge/14230 ?17:34
salgadoleonardr, I'll take it17:35
salgadoleonardr, how big is it?17:35
leonardrsalgado: about 600 lines17:36
=== deryck[lunch] is now known as deryck
mrevellAnyone want to review the what's new branch for 3.1.10? Just text changes. https://code.edge.launchpad.net/~matthew.revell/launchpad/whatsnew-3110/+merge/1423117:40
mrevelladeuring, ^^^ ?17:41
adeuringmrevell: sure17:41
mrevellthanks adeuring17:41
leonardrsalgado: sorry to run when i have a branch in the queue but i'm really hungry and must go fetch lunch. if you run into a blocker just drop my branch until i get back17:41
=== leonardr is now known as leonardr-afk
salgadoleonardr-afk, sure, no worries17:42
adeuringmrevell: r=me17:49
mrevellthanks adeuring!17:50
=== beuno-lunch is now known as beuno
salgadoleonardr-afk, the diff in your MP has a bunch of conflict markers18:04
salgadoabentley, don't MPs show when there are conflicts anymore?18:04
abentleysalgado: The automatic preview diffs don't record whether there are conflicts.18:05
salgadoabentley, but MPs used to say something like '9 conflicts' in red, at the top, no?18:06
abentleysalgado: That was generated by an external script, "MAD".18:07
salgadooh, right.  and we don't use that anymore?18:07
abentleysalgado: I'm not sure whether it's still running, but the diffs are being generated by LP now.18:08
=== leonardr-afk is now known as leonardr
leonardrsalgado, the version i'm pushing now has the conflicts resolve19:19
leonardrd19:19
salgadoleonardr, did your push finish?  I got conflicts when merging your branch19:51
leonardrsalgado: yes, version 7719:53
salgadoleonardr, it doesn't seem to be there: https://code.edge.launchpad.net/~leonardr/launchpadlib/trusted-workflow-tests-219:53
leonardrah, the name is different19:54
leonardrpushing now19:54
leonardrsalgado: try again19:55
leonardrsalgado: the reason the method names are the way they are is that i'm using python naming standards, not launchpad naming standards19:55
salgadoleonardr, right, then it's the other methods that ought to be renamed?19:56
salgadogetSingleCharInput and pressEnterToExit19:56
salgadoleonardr, also, what's the reason for this change:20:01
salgado-        self.web_root = web_root20:01
salgado+        self.web_root = uris.lookup_web_root(web_root)20:01
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
salgadoleonardr?20:13
=== adeuring changed the topic of #launchpad-reviews to: on call: - || reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/
=== salgado is now known as salgado-afk
leonardrsalgado: sorry for delay20:39
leonardruris.lookup_web_root() lets you specify "staging" instead of "https://staging.launchpad.net/"20:39

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