/srv/irclogs.ubuntu.com/2010/05/17/#launchpad-reviews.txt

=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [noodles775,wgrant,wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775Hi intellectronica! The first one in the queue there for me is: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-new-table-5/+merge/25239 when you've time.09:22
noodles775(just see the note about conflicts with db-devel on the MP... you can branch it rather than merge to run the tests).09:23
intellectronicanoodles775: yes, looking at it already09:23
noodles775intellectronica: great, thanks.09:23
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: noodles775 || queue: [wgrant,wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
intellectronicanoodles775: r=me09:53
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [wgrant,wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
noodles775Thanks intellectronica09:54
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: wgrant || queue: [wgrant, noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: wgrant || queue: [noodles775, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
jelmer_hi intellectronica10:08
jelmer_intellectronica: Can I add another branch to the queue?10:08
intellectronicajelmer_: sure10:08
=== jelmer_ changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: wgrant || queue: [noodles775, sinzui, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
jelmer_thanks :-)10:09
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: sinzui || queue: [noodles775, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [noodles775, sinzui, jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
intellectronicanoodles775: so the next in line is 567922-binarypackagebuild-new-table-6 ?11:08
noodles775intellectronica: indeed, thanks.11:08
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: noodles775 || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
adiroibanhenninge: hi. was this branch landed https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285 ?11:23
henningeadiroiban: no, don't think so. That was probably due to ec2 problems we were (are?) having.11:24
henningeadiroiban: can you merge the current devel, please, before I try again?11:25
adiroibanhenninge: yep. I will let you know when the merge was pushed.11:25
henningethanks11:26
adiroibanhenninge: merge pushed. no conflicts.11:56
henningeadiroiban: cool, I'll land it11:57
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
intellectronicagimme some more soyuz branches. ---- yeah!12:04
jelmer_:-)12:07
jelmer_it's more registry than soyuz I think12:07
intellectronicayes. looks fine too.12:09
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== adiroiban changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: - || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
adiroibandanilos, henninge, jtv : I have created a MP for the first step in having a POTemplate API. Maybe you would like to take a look https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525371/+merge/25423 :)12:14
jtvadiroiban: I can take that if you like12:15
adiroibanjtv: feel free to to it :)12:15
henningejtv: Adi and I had a pre-imp discussion about this at UDS.12:16
adiroibanadeuring: hi. Is there anything I need to do for having this branch approved and landed https://code.edge.launchpad.net/~adiroiban/launchpad/bug-402235/+merge/23973 ?12:38
adeuringadiroiban: nothing than reminding me. Sorry, I tried to get the branch in for the last release, but the ec3 test hung unfortunately. And I was on holidays last week, was conpletel offline for a few days. I'll stat the ec2 test now again.12:40
adiroibanadeuring: don't worry :) it is not a critical bug.12:41
=== barry` is now known as barry_
=== mrevell is now known as mrevell-lunch
=== jelmer_ is now known as Guest6486
=== mrevell-lunch is now known as mrevell
=== abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -,- || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
adiroibandanilos: are there any plans for allowing project maitainers to add custom language codes? this can help with fixing bug 48713714:08
mupBug #487137: Allow Rosetta admins to create custom language codes <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/487137>14:08
danilosadiroiban, not at this time, no... I am not sure that's a good idea yet, but it's what we should aim for in the future (exposing people to more and more unfinished features would make LP only harder to use)14:10
adiroibandanilos: ok14:10
=== Ursinha_ is now known as Ursinha
cody-somervilleAnyone interested in doing a review? :-)14:39
cody-somervillehttps://code.edge.launchpad.net/~cody-somerville/launchpad/create-ppa-via-api/+merge/2531914:39
cody-somervilleabentley, intellectronica: ^^ :-)14:53
intellectronicacody-somerville: otp, but can look at it after that14:53
cody-somervillekk14:54
danilosbigjools, this is logging I want to add to our jobs on the build slave, any comments? https://code.edge.launchpad.net/~danilo/launchpad/bug-580345/+merge/2541614:56
danilosjtv, henninge: ^^15:00
intellectronicacody-somerville: oright, the ppa api branch?15:00
cody-somervillecreate-ppa-via-api, yup15:01
intellectronicacody-somerville: is it really ok to create a ppa with just a name?15:01
intellectronicai mean, is it the same in the web ui?15:02
bigjoolsno it's not15:02
cody-somervilledisplayname looks optional15:02
intellectronicacody-somerville: there you go15:02
=== Guest6486 is now known as jelmer_____
intellectronicalooks?15:02
henningebigjools: huh?15:02
bigjoolshenninge: huh?15:03
henningebigjools: what question are you replying to?15:03
henningedanilos: looks good to me.15:03
* bigjools sees danilos' message and looks15:03
cody-somervillebigjools, are you says its not okay to create a PPA with just a name?15:04
cody-somerville*saying15:04
bigjoolscody-somerville: ideally no15:05
intellectronicacody-somerville: i would rename the ubuntu variable to distribution. it's only a cosmetic change, but it means that if you ever want to start handling other distros it will be more natural. you can put the xxx comment near where you assign ubuntu to the distribution variable15:05
bigjoolsotherwise the +ppas page looks crap15:05
cody-somervillebigjools, It doesn't use the name if displayname is not set?15:05
intellectronicacody-somerville: i'm a bit concerned that this will be open to misuse. how expensive is it to create new ppas? should we maybe export the method for removing them, so that if someone creates too many they can delete them easily?15:06
intellectronicabigjools: consider this too ^^^15:07
cody-somervilleThere is no cost to creating a new PPA.15:07
cody-somervilleand yes, I intend to export the ability to delete as well.15:07
bigjoolsthere's always a cost :)  intellectronica, I said that this was ok since we added deletion now15:07
intellectronicacody-somerville: i'd feel more comfortable if you said that the cost is very low :)15:07
intellectronicabigjools: you mean there's already an api for deleting them, or that you can delete them using the web interface?15:08
bigjoolsdanilos: I just added a comment to your MP15:08
bigjoolsintellectronica: just the web interface ATM, but as Cody said he's exporting that too15:08
intellectronicacool15:09
cody-somervilleintellectronica, Sorry, there wasn't enough significant digits in my calculation to provide that level of precision :P15:09
intellectronicais there a conclusion about whether displayname and description are really optional?15:09
intellectronicacody-somerville, bigjools: ^^^^^15:10
intellectronicaother than that the branch is fine15:10
* bigjools looks at branch15:10
danilosbigjools, nooooo, anything but a comment!15:10
danilosbigjools, it's there because I played with different approaches to logging (ideally I'd use Launchpad logger, but didn't want to think about how I get that installed onto build slaves as well)15:11
cody-somervilleintellectronica, displayname in not optional in web UI but description is15:12
bigjoolscody-somerville. intellectronica: there's some refactoring needed15:13
bigjoolsthe code is slightly duplicating the save_action() in the browser code15:13
* cody-somerville nods.15:14
cody-somervilleI was thinking the same thing15:14
bigjoolscody-somerville: and in fact it misses a bunch of validation15:14
intellectronicacody-somerville: if you can make the web code use the same model code for doing that it would be good15:14
bigjoolssee also validate() in the browser code15:15
intellectronicacody-somerville, bigjools: i think i'll leave you to it, then ;)15:15
bigjoolsI'll comment on the MP15:15
cody-somervillebigjools, I did look at validate.15:15
cody-somervillebigjools, a lot of it isn't necessary15:16
bigjoolscody-somerville: why?15:16
cody-somervillebigjools, because that view is a hack15:16
cody-somervillebigjools, its different depending if you're creating your first PPA or if you're creating a second PPA.15:16
bigjoolscody-somerville: the validation on the form takes that into account15:17
bigjoolsbut your new code does not15:17
bigjoolsit also doesn't check for duplicate names15:17
cody-somervillebigjools, 1. and isn't necessary since you're always supplying the name whereas the form does not15:18
cody-somerville2. It'll error out if you try that which seems fine to me since it can't provide any more meaningful feedback via the webservices API to my understanding.15:18
bigjoolscody-somerville: that doesn't matter, you still need to check that it's the first PPA and then check if the CoC is signed15:18
cody-somervillebigjools, Thats what the discussion was about with jml on Friday night.15:19
bigjoolsre. (2), you're not testing that so you don't know by definition ;)15:19
bigjoolsyes, we said we'd think about ToS-ing the Coc or something15:19
cody-somervillealso, the browser class doesn't check if the CoC is signed either15:20
cody-somervilleand we can't check if the PPA ToS is signed since its not actually recorded15:20
bigjoolscody-somerville: it checks to see if you have accepted the ToS15:20
cody-somervillebigjools, using a hack. Its not actually recorded.15:21
cody-somervillebigjools, which is why I added a note in the function docstring that using the API constitutes accepting the PPA ToS.15:21
bigjoolshack?15:21
bigjoolsok that's fine I think15:21
cody-somervillebigjools, Yes. It doesn't record it. If a PPA exists, it assume the ToS is accepted. If no PPAs exist, then it'll try creating the default PPA and provide that different view that has that checkbox.15:22
bigjoolsbut, I think you should add more tests where it adds the same name twice15:22
bigjoolsI suspect it'll generate an OOPS15:22
bigjoolsand we don't want that15:22
cody-somervillebigjools, I think thats fair feedback.15:22
bigjoolsI'll write on the MP15:23
cody-somervillebigjools, ty15:23
cody-somerville:-)15:24
bigjoolscody-somerville: np, thanks for the change15:24
bigjoolshow was the hangover on Saturday? :)15:24
cody-somervillelol. didn't drink enough for one (un)fortunately.15:25
cody-somervillebigjools, how about yourself?15:27
bigjoolscody-somerville: you were pretty hammered on Friday :)15:27
bigjoolsI was tired, slept like a baby on the train hime15:27
bigjoolshome15:27
BjornTcody-somerville, intellectronica, bigjools: don't forget to add tests for making sure who has permission to call the createPPA() method.15:29
cody-somervilleShouldn't there already be tests for that?15:30
bigjoolscody-somerville: since you added the method, no :)15:30
bigjoolsyou need a unit test for it15:30
BjornTcody-somerville: yes, the one who added the method should have added tests for that ;)15:30
cody-somervillecreatePPA is just a wrapper around new on ArchiveSet15:31
bigjoolsstill needs a test dude15:31
bigjoolsyou should use launchpadlib in the story test too15:32
bigjoolsI updated the MP15:32
BjornTcody-somerville: also think about the documentation. now you're saying that anyone can call createPPA. they shouldn't have to try to call it to find it out.15:33
cody-somervillevery well.15:35
bigjoolsso, Ubuntu Software Centre fails under Kubuntu :/15:38
=== deryck is now known as deryck[lunch]
=== barry_ is now known as barry
EdwinGrubbsBjornT, have you had a chance to look at the schema changes in https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-535430-needspackaging-timeout-part1/+merge/2535916:16
gmbintellectronica, abentley: I have another branch for your queue if that's okay: https://code.edge.launchpad.net/~gmb/launchpad/show-oops-ids-bug-564686/+merge/2544216:23
abentleygmb, I'll take it.16:24
gmbabentley, Thank you.16:24
BjornTEdwinGrubbs: no, i haven't. i'll look now16:24
=== adiroiban changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -,- || queue: [adiroiban(bug-570899), adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -,gmb || queue: [adiroiban(bug-570899), adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
abentleygmb, is it possible to use the same code path for generating oops URLs that other code does?16:31
gmbabentley, I looked into that, and tried to reuse the code in webapp/tales.py, but there didn't seem to be a simple way to do use it (I thought there would be a formatter, but apparently not). I'll have a dig around, though, see if I can find anything else.16:33
=== intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: adiroiban(bug-570899),gmb || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
intellectronicaadiroiban: you've got several jslint notices. what about them?16:35
intellectronicaadiroiban: also, why can't you test?16:37
=== matsubara is now known as matsubara-lunch
gmbabentley, I can't find anything reusable so far; will keep looking but I think we'd be better off having a formatter do the work for us. I might as well fix that in this branch.16:41
abentleygmb, that would be great if you don't mind.16:41
gmbabentley, Sure, no problem.16:41
BjornTEdwinGrubbs: remind me, what is section, and can it change between different series?16:57
EdwinGrubbsBjornT, section for debs are "apps", "graphics", etc.16:58
=== deryck[lunch] is now known as deryck
BjornTEdwinGrubbs: ok. let's say a package was in "apps" in karmic, and "graphics" in lucid. first a new release is uploaded to lucid, and then one is uploaded to karmic. what value will DistributionSourcePackage.section have?17:01
BjornTEdwinGrubbs: also, what happens if a PPA package is uploaded?17:03
EdwinGrubbsBjornT, that's an interesting point. The trigger which caches it, gets the latest one. I'm not sure what the current behavior is for the page that gets the data directly from the SPPH table.17:04
BjornTEdwinGrubbs: so, i would suggest discussing this patch with bigjools17:04
=== gary_poster is now known as gary-lunch
EdwinGrubbsBjornT, I forgot, I need to select only PRIMARY packages to eliminate PPAs.17:05
bigjoolsEdwinGrubbs: why are you using a trigger?17:06
bigjoolsEdwinGrubbs: BjornT also has a good point about which series' data you're caching17:10
EdwinGrubbsbigjools, since we would like to consolidate the DistributionSourcePackage class and the DistributionSourcePackageInDatabase class, the object will need to exist in the db by the time the code will use it. This could be done with a cronjob, but that actually seems like it would have more intensive queries, since it would have to query all the SPPHs for a distr+spn in order to find the one with the latest date.17:10
bigjoolsI suspect it should only be the one that's marked 'current' for each distro17:11
bigjoolsSince DSP is going to start getting published-specific data, you need to define which series that will be for, and how it gets updated when a new series is opened17:14
bigjoolsI think you can do that without a trigger or cron job17:15
EdwinGrubbsbigjools, where is a distroseries specified as current?17:15
bigjoolsits status17:16
EdwinGrubbsbigjools, when you say that I can do that without a trigger or cronjob, do you want the class to automatically create the DSP record whenever it is requested?17:18
bigjoolsEdwinGrubbs: I think that's a good idea, yes.  It would only happen at package upload time.17:19
bigjools(which is never a webapp transaction)17:19
bigjoolsso when creating a publishing record, you can check to see if it's the right series and then update/create the DSP17:20
bigjoolsthis will also be good for Soyuz when we start caching stuff :)17:20
EdwinGrubbsbigjools, which script creates the spph record? I assume it's triggered by dput.17:21
bigjoolsand it's also an easy check to see if it's the PRIMARY archive17:21
bigjoolsEdwinGrubbs: let me check where the best place to do this is17:21
adiroibanintellectronica: back. I don't know how to fix those jslint.17:23
bigjoolsEdwinGrubbs: see lib/lp/soyuz/model/publishing.py17:24
bigjoolsEdwinGrubbs: IPublishingSet.newSourcePublication()17:25
adiroibanintellectronica: as for LEFT and RIGHT keys, it looks like Windmill does not support trigering such events17:25
bigjoolsEdwinGrubbs: you need to check that the archive.purpose = PRIMARY, then you can see if the DSP exists or not and create/update17:26
bigjoolsthat's much easier to test than a trigger :)17:26
adiroibanintellectronica: here is the windmill bug http://github.com/windmill/windmill/issues#issue/3317:26
bigjoolsEdwinGrubbs: have you figured out which series you need to cache data for?17:27
=== abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: adiroiban(bug-570899), - || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
bigjoolsbearing in mind development is newer than current17:27
=== abentley is now known as abentley-lunch
bigjoolshaving said that, section is not likely to change17:28
intellectronicaadiroiban: ok, let me see if i can get rid of the jslint errors. we can't land with them.17:31
adiroibanintellectronica: I had those errors in other branches and Danilo said that it is ok and this is the way YUI3 works17:35
=== matsubara-lunch is now known as matsubara
intellectronicaadiroiban: i rather check first, and at least register the problem somehow. it's not a good idea to keep jslint errors in the codebase.17:37
danilosadiroiban, I don't remember saying so unless if it was a branch where you didn't touch the faulty JS which I don't want to hold you responsible for :)17:37
EdwinGrubbsbigjools, I don't know for which series the section should be cached. Maybe sinzui knows.17:41
intellectronicaadiroiban: also, i think that at this point it would make sense to assign the magic numbers to CONSTANTS, but I won't ask you to do it because i see that you're just extending legacy code.17:42
sinzuiseries cache>17:42
bigjoolsEdwinGrubbs: it's an important question since this table will be useful beyond your use case I think17:42
sinzuiIs someone watching my keystrokes as I addtal:content="cache:public, 1 hour"17:42
sinzuito series templates?17:42
bigjools\m/17:42
adiroibanintellectronica: I will create those contants as it should improve the code readability17:43
intellectronicaadiroiban: excellent, thanks!17:43
sinzuibigjools, I think I am working on the issue you are talking about. I am working on the timeouts for distroseries and milestones that dominate oops17:44
bigjoolssinzui: I am checking over Edwin's branch, I guess you're both working on this?17:45
gmbabentley-lunch, I've added a formatter for oops IDs and some tests to go with it. I've pasted a diff in the MP for your perusal.17:46
sinzuibigjools, I made the initial proposal. and I liked EdwinGrubbs suggested changes. I have not looked at the branch though17:46
sinzuibigjools, I suppose you are remarking that once a series is released, the packages should not change.17:48
sinzuiis backports main?17:48
intellectronicaadiroiban: that code is a bit weird, no wonder jslint complains17:49
bigjoolssinzui: well a couple of things, 1. we should do it in code not in a trigger (soyuz will benefit from cache publishing data in places so I want to prepare for that), 2. yes a released series won't change17:49
intellectronicadanilos: ^^^^^17:50
bigjoolssinzui: backports is a pocket, not sure what you mean?17:50
bigjoolsyou mean the primary archive?17:50
sinzuibigjools, thanks for your incite17:50
sinzuiyes I mean primary archive17:50
bigjoolsso yes -backports is the primary archive17:51
sinzuiyuck17:51
bigjoolsproblems?17:51
sinzuium, no actually. I had a momentary lapse of reason17:52
bigjoolsI get that all the time :)17:52
* sinzui is still thinking of series and milestones17:52
EdwinGrubbssinzui, on which page will you be querying the cached section from the DSP table? It doesn't look like that is used on the +needs-packaging page.17:55
adiroibanintellectronica: let me rewrite it. what do you consider to be weird ? :)17:55
intellectronicaadiroiban: i don't understand what is them `stem` second argument that the even handlers receive17:57
intellectronicaadiroiban: oh, is it the last argument you pass to on()?17:58
sinzuiEdwinGrubbs, I think the root page is the only user of memcached. I am wrapping portlets.17:58
sinzuiEdwinGrubbs, I have not tested memcached with a batched listing. I do not think it will work17:58
adiroibanintellectronica: let me rewrite that code as I realise it is just stupid :)18:05
intellectronicaadiroiban: i have a solution for the first batch of lint errors, but it ain't pretty :)18:05
intellectronicaadiroiban: what were you thinking of doing?18:06
adiroibanintellectronica: those functions are adapters for toggleWidget()18:06
adiroibanas the YUI3 will pass the event as the first argument18:07
adiroibanI can not use them directly with Y.on18:07
=== gary-lunch is now known as gary_poster
intellectronicaadiroiban: yeah, got it. the solution i have is to enclose the handlers in a closure. it isn't necessary in this case, since you're not capturing anything but using yui's event machinery to pass an argument, but jslint is correct to make that suggestion, and a nicer solution would require refactoring the code.18:09
EdwinGrubbssinzui, What does memcached have to do with the DistributionSourcePackage.section column?18:09
intellectronicaadiroiban: a nicer solution might be to move the body of the loop into a separate function18:09
sinzuiEdwinGrubbs, memcached+tales caches page fragments. Which page/part do you want me to test?18:10
adiroibanintellectronica: i was thinking to create those adapter functions outside the loop18:11
intellectronicaadiroiban: yes, that's also a possibility. whichever you prefer.18:12
sinzuiEdwinGrubbs, memcached+tales does include the query string in the key. I think we can cache batches18:21
EdwinGrubbssinzui, I'm adding the section column to the DistributionSourcePackage table to cache the data from the SPPH table. I think you told me to do that on Friday. I don't know where the query is that we are trying to speed up. This should be unrelated to memcached.18:21
sinzuiEdwinGrubbs, yes, it is unrelated. To exclude language-packs, we use the section which is know in spph or in spr. I used spph in the branch I am landing today18:23
sinzuiEdwinGrubbs, yes, it is unrelated. To exclude language-packs, we use the section which is know in spph or in spr. I used spph in the branch I am landing today18:23
=== intellectronica changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
=== EdwinGrubbs_ is now known as foo1000
leonardrabentley, i'd like to add https://code.edge.launchpad.net/~leonardr/lazr.restful/understand-mod-compress/+merge/25454 to the review queue19:14
marsgary_poster, ping, have a moment to review https://code.edge.launchpad.net/~mars/launchpad/disable-windmill-tests/+merge/25457 ?19:59
gary_postermars, ok19:59
gary_postermars, you added in TESTOPTS for theoretical correctness, yes?20:03
gary_posterthat is, no one is actually using them, it seems?20:03
marsgary_poster, it was already in there, just not used properly.20:03
marsthey are used by three targets lower down in the file20:04
gary_posterah, gotcha mars20:04
marsI was going to rename VERBOSITY to TESTOPTS myself, and was surprised to find TESTOPTS was already in there20:04
gary_posteryeah, another approach would be to rip it out, but I suppose this is safer20:05
marsusing the value of the VERBOSITY variable to pass "-vvv -t --layer" to the test process really rubbed me the wrong way.  Refactoring gene I guess :/20:05
marsgary_poster, safer, yes.  I don't know who else used VERBOSITY that way.20:05
gary_postermars, have you checked to see what happens to the test runner if you say --layer='!WindmillLayer' --layer='WindmillLayer'?20:06
marsNeeded a fast fix.  This is one step forward.20:06
marsMy guess is that --layer=!Layer --layer=Layer *will* execute the desired test suite.20:06
gary_posterwell, for a fast fix, maybe leaving TESTOPTS out of the picture would be wiser.  Otherwise I have questions about it20:06
marsBut no, I have not explicitly tested that :)20:06
marsgary_poster, oh?  What questions?20:07
gary_posterfor instance--if you are right (which would be my guess) then I would suggest that ``bin/test $(VERBOSITY) $(TESTOPTS) --layer=WindmillLayer`` should instead be ``bin/test $(VERBOSITY) --layer=WindmillLayer $(TESTOPTS)`` so that TESTOPTS can actually override things20:07
marsgary_poster, I treat TESTOPTS like CFLAGS.  VERBOSITY is independent of the two.20:07
marsgary_poster, ok, interesting.  That woudl have come up regardless if I used VERBOSITY or TESTOPTS.20:08
gary_posteryeah, calling it TESTOPTS is more correct, and makes the possible problem clearer.20:09
gary_postermars, I'll approve, simply asking that you consider my question...possibly later. :-)20:09
mars+120:09
marsgary_poster, I'll send a post to the list when this lands.20:10
gary_postermars, ok.  approved.20:10
marsa post about the suite disablement20:10
gary_posterright20:10
marsthanks gary_poster20:10
gary_postermars, also note that when this branch lands the linked bug will be marked fix committed I think.  If so, I'm guessing (?) you'll want to manually change it back, since this is a bandaid not a fix.20:11
marsgary_poster, yep!  Had to do that a few times now.  The QA scripts also do that.20:12
marsI'm working against the grain!20:12
gary_poster:-)20:12
=== abentley-lunch is now known as abentley
=== matsubara is now known as matsubara-afk
leonardrabentley, i don't know if you're still reviewing but i've also got a launchpad follow-up to my earlier lazr.restful branch21:16
leonardrhttps://code.edge.launchpad.net/~leonardr/launchpad/understand-mod-compress21:16
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: leonardr || queue: [adiroiban(bug-525371)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
abentleyleonardr, did you mean to target your branch at db-devel, rather than devel?21:19
leonardrabentley: i don't think so, i generally target devel--is devel closed atm or something?21:20
abentleyleonardr, devel is not closed.  You have proposed for landing against db-devel.  This means that some other changes from devel are showing in your diff.21:21
leonardrabentley: i misunderstood your question21:21
leonardri thought you were saying "you targeted your branch at devel, but you probably meant db-devel" rather than vice versa21:21
abentleyleonardr, ah, thought so.21:22
leonardrabentley, i'm redoing the proposal21:22
abentleyleonardr, thanks.21:22
leonardrdone21:23
abentleyleonardr, there should be two blank lines before a new header in a docstring.21:39
abentleyleonardr, also, the differences between '"foo-gzip"' and '"foo"' info '"foo"-gzip' are pretty subtle.  It would be nice to mention them both at the same time.21:41
abentleye.g.:  mod_compress may turn '"foo"' into '"foo"-gzip' or '"foo-gzip"'.  We treat both forms as the same.21:42
abentleyalso, you say "info" where you mean "into".21:43
=== abentley 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
leonardrgary: abentley reviewed my launchpad branch but not the lazr.restful branch it uses. would you take a look?22:03
leonardrhttps://code.edge.launchpad.net/~leonardr/lazr.restful/understand-mod-compress/+merge/2545422:03
gary_posterleonardr: ack, looking22:03
adiroibanintellectronica: i have updated the JS code adding constanst for key codes and fixing jslint warnings.22:13
gary_posterleonardr: approved, no comments.  thank you22:13
leonardrgary, great22:13

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