| gary_poster | lifeless: hey. bad news: I have a 1336 line MP. possibly good news: it is for ++profile++ . https://code.edge.launchpad.net/~gary/launchpad/lsprof-on-demand/+merge/33849 if you want to look at it, or I'll try to get someone to look at it tomorrow. | 00:03 |
|---|---|---|
| lifeless | gary_poster: thanks | 00:16 |
| lifeless | gary_poster: it still drops the profile output in the previous place ? | 00:19 |
| gary_poster | lifeless, yes. configurable. | 00:20 |
| lifeless | I have a suggestion | 00:20 |
| gary_poster | cool; | 00:20 |
| lifeless | if we used a feature flag | 00:20 |
| lifeless | with a scope that matches users in canonical-launchpad | 00:21 |
| lifeless | it would be less risky to have on on staging | 00:21 |
| lifeless | and also permit turning it on and off without rebooting the appserver | 00:21 |
| lifeless | you could do this without any schema changes | 00:22 |
| lifeless | its just a thought | 00:22 |
| gary_poster | lifeless, that sounds great. want that in a separate branch? | 00:23 |
| lifeless | something else that would be really lovely | 00:24 |
| lifeless | is a kcachegrind file on disk (and oops on disk); and a way to download them (again, enabled for specific users/groups by flags) | 00:24 |
| lifeless | gary_poster: in end_request, you've added a bunch of vertival white space | 00:26 |
| lifeless | this is a matter of taste, but I find that very jarring when reading a function | 00:26 |
| lifeless | I prefer either no VWS or separate functions. | 00:26 |
| gary_poster | lifeless, download: I maybe could make ++profile++download download the kcachegrind. | 00:27 |
| lifeless | (PEP8 says to use it for 'significant regiosn's (misquoting) | 00:27 |
| gary_poster | VWS: OK. I liked it here obviously, but it was very...organic. Happy to remove it. | 00:28 |
| lifeless | we don't need docstrings on tests btw | 00:28 |
| lifeless | or if we do its nuts :) | 00:29 |
| lifeless | as long as the intent is clear | 00:29 |
| lifeless | if its vague, a docstring or comment to make it clear is good in its own right | 00:29 |
| lifeless | 1334+ | 00:29 |
| lifeless | 1335+def test_suite(): | 00:29 |
| lifeless | 1336+ return unittest.TestLoader().loadTestsFromName(__name__) | 00:29 |
| lifeless | thats not needed now | 00:29 |
| lifeless | gary_poster: approved | 00:31 |
| gary_poster | lifeless: awesome thanks | 00:32 |
| lifeless | gary_poster: there was one other thing | 00:42 |
| lifeless | the 'tests are not setting stuff up so ignore it' squicks me; I realise why, and the pragmatic need, but I'd like to be able to put a ratchet on those tests and eliminate the badness. | 00:43 |
| gary_poster | lifeless: not clear what you mean yet? | 00:43 |
| gary_poster | oh! | 00:43 |
| gary_poster | yeah that was annoying | 00:44 |
| gary_poster | what do you have in mind for ratcheting? | 00:44 |
| lifeless | well | 00:44 |
| lifeless | depends on how widespread it is I guess | 00:44 |
| lifeless | if its only a few root causes, like some test helpers, we could just do warning.warn() in a branch, and iterate on the branch till its done. | 00:45 |
| lifeless | if its more substantial, perhaps starting the test suite could empty a file | 00:45 |
| lifeless | and each occurence be logged (one line per) | 00:45 |
| lifeless | and the total number of lines reported at the end | 00:45 |
| gary_poster | between a few and several. I have to run though. Please copy that in to the MP and I'll act on it tomorrow. | 00:45 |
| lifeless | lint could report if that file is longer than N | 00:45 |
| === Ursinha is now known as Ursinha-afk | ||
| mwhudson | thumper: care to look at https://code.edge.launchpad.net/~mwhudson/launchpad/cross-product-spec-links-bug-3552/+merge/33866 ? | 05:42 |
| thumper | mwhudson: on a friday afternoon? | 05:43 |
| thumper | mwhudson: just before beer? | 05:43 |
| mwhudson | thumper: it'll make the beer taste better | 05:43 |
| mwhudson | mm, proportionally fonted diffs | 05:44 |
| thumper | mwhudson: line 23 of the diff | 05:46 |
| mwhudson | thumper: lolz | 05:47 |
| mwhudson | thumper: fix being pushed | 05:48 |
| lifeless | hmm, exported_as doesn't work if there is an attribute with the same name that isn't exported. | 06:23 |
| lifeless | echannel | 06:23 |
| mwhudson | thumper: thanks | 06:29 |
| henninge | jtv: fancy a quick review? | 08:07 |
| henninge | https://code.edge.launchpad.net/~henninge/launchpad/recife-pofile-creation/+merge/33800 | 08:07 |
| jtv | henninge: coming | 08:08 |
| henninge | jtv: thanks | 08:08 |
| jtv | henninge: in this test you say "shared" POTemplates. IIRC Danilo has been calling them "sharing" POTemplates. | 08:09 |
| jtv | Also, "a <thing> has the same name" is a bit nonsensical—same name as what? | 08:10 |
| henninge | jtv: right, I copied that comment without thinking. | 08:10 |
| jtv | That explains the old-style setup as well. | 08:10 |
| henninge | And I have always ued 'shared'. | 08:10 |
| henninge | jtv: what's 'old-style' ? | 08:11 |
| jtv | I think "shared" is misleading here, because the POTemplate isn't shared. It shares things with other POTemplates. | 08:11 |
| henninge | I see | 08:11 |
| jtv | Old-style is relying on sample data (hoary and warty) instead of creating fresh data. | 08:11 |
| jtv | (*) in this context. :) | 08:12 |
| jtv | After that it suddenly goes into the new style. | 08:12 |
| henninge | jtv: Good point. I will change that. | 08:12 |
| jtv | Also, you can shortcut some of the setup by letting the first self.factory.makeSourcePackage cal create your SourcePackageName for you. | 08:13 |
| jtv | *call | 08:13 |
| jtv | Probably your distroseries as well. | 08:14 |
| jtv | Otherwise it's just too much setup to read through! | 08:15 |
| jtv | henninge: in the comment for test_pofile_creation_shared_in_ubuntu, I assume "POTemplate of project" means "POTemplate of a project," not some other typo such as "POTemplate or project"? | 08:17 |
| henninge | jtv: you are right | 08:20 |
| henninge | jtv: Don't think I'll get around an explicit distroseries creation because it has to be for Ubuntu, does it not? | 08:35 |
| jtv | henninge: ouch—I thought that's what we got. The code you're testing here is probably one of the very few places that care about the difference. | 08:37 |
| henninge | jtv: Also, in the second test I use sourcepackagename so often that I'd put it in a variable anyway and creating it explicitely keeps the source packages creation symetric. | 08:41 |
| jtv | henninge: whatever works for you—just try not to throw too much text at the reader! | 08:42 |
| jtv | it's not all about the number of lines though. | 08:42 |
| jtv | A short assignment line is generally easier to read than a very long one with a method call in it and lots of mixed-case letters. | 08:43 |
| StevenK | adeuring: Hi! Got time to review a trivial MP for me? | 08:48 |
| adeuring | StevenK: sure (except for a not yet sufficient caffeine level ;) | 08:48 |
| StevenK | adeuring: https://code.edge.launchpad.net/~stevenk/launchpad/lucille-archivepermission/+merge/33873 when you're sufficently caffeinated | 08:49 |
| adeuring | StevenK: where is it? | 08:49 |
| adeuring | StevenK: so, the method newPackageUploader needs to be called by the db user 'lucille'. What does lucille do? And do we know if lucille indeed adds permissions only for people who should get them? | 09:02 |
| StevenK | adeuring: No, that isn't it, that's just set up. | 09:03 |
| StevenK | adeuring: The code that this actually affects is in part of the code that didn't change -- it adds new archivepermission rows directly into the database during initialise-from-parent | 09:03 |
| adeuring | StevenK: so, some sort of copy operationß | 09:05 |
| adeuring | sß/?/ | 09:05 |
| StevenK | adeuring: Yes, it is initialising a child distroseries from its parent | 09:05 |
| adeuring | StevenK: ah, ok. r=me then | 09:06 |
| StevenK | adeuring: Thank you! | 09:06 |
| noodles775 | adeuring: one-liner to fix the latest db-devel test-fix (when the MP diff updates): https://code.edge.launchpad.net/~michael.nelson/launchpad/db-devel-import-fix-20100827/+merge/33876 | 09:07 |
| StevenK | noodles775: Oh, if you're going to fix db-devel, I noticed a .THIS file crept in, can you bin it? | 09:08 |
| noodles775 | StevenK: sure | 09:08 |
| adeuring | noodles775: approved | 09:09 |
| noodles775 | adeuring: thanks. I've just pushed 9708 which deletes the .THIS file StevenK mentioned. | 09:10 |
| adeuring | noodles775: well, that's fine, I think ;) | 09:10 |
| noodles775 | StevenK: my landing will remove it from db-devel, but it's actually committed in devel. | 09:10 |
| noodles775 | Thanks adeuring | 09:11 |
| StevenK | Rah | 09:11 |
| StevenK | noodles775, adeuring: I have a branch ready for devel, shall I kill that .THIS file in it? | 09:12 |
| noodles775 | StevenK: Sounds good. | 09:12 |
| adeuring | StevenK: makes sense | 09:12 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | adeuring, Could you review https://code.edge.launchpad.net/~gmb/launchpad/lp-devs-can-reset-watches/+merge/33796 for me please? | 10:28 |
| adeuring | gmb: sure | 10:29 |
| gmb | Thanks | 10:29 |
| noodles775 | stub or lifeless: do either of you have time to review a 70line db patch? https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-schema/+merge/33515 | 10:33 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Hi again adeuring, when you've time, can you please take a look at https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-basic-model/+merge/33885 | 10:34 |
| adeuring | noodles775: sure | 10:35 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: gmb || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| adeuring | gmb: r=me; 1 nitpick | 10:56 |
| gmb | adeuring, Thank you. | 10:57 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | adeuring, foo*Condition is the usual naming idiom for methods used to decide whether or not we can display an action. Maybe showResetActionCondition() would be clearer. | 10:59 |
| adeuring | gmb: right, that's better! | 10:59 |
| gmb | Right, I'll do that, then. | 10:59 |
| gmb | Thanks adeuring. | 10:59 |
| stub | noodles775: Reviewed. The derived distro series calling its parent the derived distro series seems a problem. | 10:59 |
| noodles775 | stub: thanks... looking. | 11:00 |
| stub | Or I think I'm just confused.. | 11:00 |
| stub | oic. This is the difference, it points to the derived distro series, there is some other link that points to the parent series. | 11:00 |
| noodles775 | Yes, there's a reference from IDistroSeries.parent_series... which is why we only need to reference the one (derived) series here. | 11:01 |
| stub | DistroSeries.parent_series already exists. | 11:01 |
| stub | Ok... rereviewing :) | 11:01 |
| noodles775 | Correct. | 11:01 |
| noodles775 | Thanks.... just reading your other comments :) | 11:02 |
| noodles775 | stub: Why would we want delete permissions? At the moment I'm planning that these differences will end in a resolved state, but we wouldn't want to delete them until the derived distroseries is itself deleted (if it ever is). | 11:04 |
| stub | The derived distribution has a patched version of firefox. Tomorrow it decides that it should just use the parents version of firefox. | 11:05 |
| noodles775 | Great, so assuming the parents version has a higher revno, they upload it, this record is marked as resolved and the activity log is updated, but the record is not deleted, as tomorrow the parent might upload a new version, and the previous comments/activity is still useful. | 11:07 |
| stub | ok | 11:07 |
| stub | How big do you think the comments and activity will grow? | 11:08 |
| stub | It will be a pain to split out into a structured format later. | 11:08 |
| stub | If it is append only, storing it as a single text blob isn't ideal. If it is more a whiteboard or a copy of a text file stored in a branch, that would be fine I guess. | 11:10 |
| noodles775 | On average 4 or 5 lines. If a particular difference goes through resolved->needs attention-> resolved it would be a few more. | 11:10 |
| stub | But over time | 11:10 |
| noodles775 | Yes, it is just like a whiteboard. | 11:11 |
| stub | ok | 11:11 |
| noodles775 | Yep, so if overtime it went through that cycle (resolved->needs attention->resolved) *lots*, there would be 2 lines for each change. | 11:11 |
| noodles775 | Do you think its worth adding a separate comment model? | 11:11 |
| noodles775 | (from memory the code/bugs guys made the comment model re-usable... I'll check). | 11:12 |
| noodles775 | stub: oh, and I think I misunderstood your comment above. I am making the activity_log append only. | 11:14 |
| stub | So we would probably be better off with a separate table containing the activity, one row per addition. Or even a link table between distroseriesdifference and message same as our other comment spools. | 11:17 |
| noodles775 | Yep, just looking at the message table... I'll do that then. | 11:18 |
| noodles775 | Thanks stub. | 11:18 |
| stub | That gives you the infrastructure for attachments, email interfaces etc. if we want them in the future. | 11:19 |
| noodles775 | Yeah, given that the table already exists, it sounds crazy not to use it. | 11:20 |
| jtv | noodles775: this definitely needs code review and it may also need UI review: https://code.edge.launchpad.net/~jtv/launchpad/bug-517700/+merge/33888 | 11:39 |
| jtv | (Sorry for the diff size… I didn't exactly aim to use up the entire budget) | 11:39 |
| jtv | Drat. Doesn't linkify the bugs in = Bugs 484375, 517700 = | 11:40 |
| jtv | bugs 484375, 517700 | 11:41 |
| jtv | That used to work. | 11:41 |
| jtv | noodles775: oh, hang on—there's conflict now (also neatly pulling it across the 800-line limit) | 11:41 |
| noodles775 | jtv: did you mean adeuring ? | 11:43 |
| jtv | noodles775: arrrgh, yes, sorry… an experiment in reading the topic line from the tiny low-contrast fineprint my client shows at the top of the window | 11:43 |
| adeuring | jtv: I'll look at it | 11:44 |
| jtv | adeuring: hang on, I've got some conflicts to resolve first! | 11:44 |
| adeuring | jtv: ok, ping me when the branch is ready | 11:44 |
| jtv | adeuring: MP has updated — https://code.edge.launchpad.net/~jtv/launchpad/bug-517700/+merge/33888 | 12:02 |
| adeuring | jtv: OK, let me first finish the review for noodles775, then I'll need a lunch break, then i'll look at your branch | 12:03 |
| jtv | adeuring: great, thanks. I'll probably be out by then, unfortunately | 12:03 |
| noodles775 | stub: updated with an incremental, but feel free to leave it for Monday :D Thanks. | 12:08 |
| * noodles775 lunches | 12:08 | |
| jtv | bigjools: https://code.edge.launchpad.net/~jtv/launchpad/fakelibrarian-fixture/+merge/33892 | 12:22 |
| jtv | bigjools: I thought you might like to review that one. :) | 12:22 |
| adeuring | noodles775: in test_new_non_derived_series(), you are using the factory method in assertRaises, which is IMHO quite indirect. What about testing the new() method directly? | 12:27 |
| jelmer | adeuring: Hi, can I add another mp to your queue? | 13:10 |
| adeuring | jelmer: sure | 13:11 |
| jelmer | adeuring: Thanks; the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa-qa/+merge/33804 | 13:15 |
| === jelmer changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | adeuring: Yep, I'll update. | 13:18 |
| === matsubara-afk is now known as matsubara | ||
| === Ursinha-afk is now known as Ursinha | ||
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jtv || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| * jtv chears adeuring on | 13:28 | |
| jtv | *cheers | 13:29 |
| bac | hi adeuring | 13:53 |
| adeuring | hi bac1 | 13:53 |
| adeuring | bac! | 13:53 |
| bac | hi adeuring, i'm ocr today but won't be able to help for a couple of hours. | 13:55 |
| adeuring | bac: ok, no problem | 13:55 |
| bac | thanks abel. | 13:55 |
| adeuring | (except perhaps for people needing a review after 1730UTC ;) I have an appointment this evening... | 13:56 |
| adeuring | erm, 1700UTC.. | 13:56 |
| adeuring | jtv: r=me | 14:19 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | adeuring: splendid, thank you | 14:25 |
| adeuring | welcome :) | 14:25 |
| === benji changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jelmer || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| benji | my MP is at https://code.launchpad.net/~benji/launchpad/bug-580035/+merge/33911; it's for a bug in a bug fix (found while doing QA) | 14:45 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| adeuring | jelmer: R=Me | 14:59 |
| adeuring | (no longer r=me) | 14:59 |
| adeuring | ...for this branch ;) | 14:59 |
| jelmer | adeuring: :-)) and thanks | 14:59 |
| adeuring | benji: GPGHanlder.getVerifiedSignature() can raise GPGVerificationError. I think this exception should be caught in extract_signature_timestamp() | 15:21 |
| adeuring | erm, or in ensure:sane:signaturetimestamp() | 15:22 |
| adeuring | ...or are we sure that we have a valid signature? | 15:22 |
| benji | adeuring: at the point that I call GPGHanlder.getVerifiedSignature() it has already been called successfully | 15:23 |
| benji | right | 15:23 |
| adeuring | benji: ok, thanks! | 15:23 |
| adeuring | benji: r=me | 15:33 |
| benji | danke | 15:34 |
| benji | adeuring: it says "Approve (code)" but at the top still says "Status: | 15:35 |
| benji | Needs review" | 15:35 |
| adeuring | benji: fixed | 15:36 |
| benji | thanks | 15:36 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: benji,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === salgado is now known as salgado-lunch | ||
| === sinzui changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | hi jelmer | 16:07 |
| sinzui | adeuring, bac: I have a branch for review | 16:07 |
| bac | jelmer: i'm looking at https://code.edge.launchpad.net/~jelmer/launchpad/buildmaster-enums/+merge/33893 | 16:07 |
| jelmer | hi Brad | 16:07 |
| adeuring | sinzui: I'll look | 16:07 |
| bac | at line 379 is it really necessary to do the import locally? | 16:07 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,jelmer || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| sinzui | adeuring, bac: I am going to start work on a branch to disable google maps. We are treating this as a critical issue. | 16:08 |
| bac | sinzui: ok | 16:08 |
| jelmer | bac: Thanks. 379 is a removed line here in the diff | 16:09 |
| adeuring | sinzui: the mp i should at is this one: https://code.edge.launchpad.net/~sinzui/launchpad/licenses-0/+merge/33916 ? Or the google maps branch? | 16:09 |
| jelmer | bac: 511 has a local import that I couldn't really work around. | 16:09 |
| === salgado-lunch is now known as salgado | ||
| bac | jelmer: you pushed a new branch since i started looking at it, that's why the line numbers don't match | 16:10 |
| jelmer | bac: Ah, my bad - sorry. I intended to push that to a separate branch. | 16:11 |
| bac | but in that MP line 511 is the one i was asking about | 16:11 |
| jelmer | bac: Would you like me to remove that revision or is the MP fine as is (it's a bit large) | 16:11 |
| bac | jelmer: the branch is so mechanical it doesn't matter | 16:12 |
| jelmer | bac: Ok | 16:13 |
| jelmer | bac: I can probably work around the local import by putting BUILDD_MANAGER_LOG_NAME in a separate module. manager.py seems like the best place for it though. | 16:13 |
| bac | jelmer: the local import isn't critical...just wanted to make sure there was a good reason for it. if you can remove the need without a lot of hassle that would be great. | 16:14 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: sinzui,jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | jelmer: r=bac, with some import sorting fixes. perhaps you can find henning's tool and run it? (though that would take longer than doing it by hand i suspect.) | 16:27 |
| jelmer | bac: I should get used to doing it right in the first place so I'll do it by hand. :-) Thanks for the review | 16:28 |
| bac | np jelmer. thanks for the branch. | 16:28 |
| henninge | bac: I am working on putting it into the tree ATVM. ;-) | 16:29 |
| bac | henninge: i'll happily review it. | 16:30 |
| henninge | bac: should I add tests? | 16:37 |
| henninge | ;) | 16:37 |
| bac | henninge: that would be nice, of course. but we do have lots of thing in utilities that are stand-alone tools that don't have tests. | 16:38 |
| === Ursinha is now known as Ursinha-lunch | ||
| henninge | bac: since the tool changes other files in the tree, the real test is if those changes break the test suite. | 16:39 |
| henninge | or not | 16:40 |
| bac | henninge: you've sold me | 16:40 |
| * henninge prepares MP ;) | 16:40 | |
| henninge | Actually, let me add something else first. | 16:48 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === salgado is now known as salgado-lunch | ||
| henninge | bac: Here it is: | 17:19 |
| henninge | https://code.edge.launchpad.net/~henninge/launchpad/format-imports/+merge/33926 | 17:19 |
| bac | thanks henninge | 17:22 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === adeuring changed the topic of #launchpad-reviews to: On call: -, bac || reviewing: -,henninge || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bac | henninge: i don't understand the comment describing FIRST | 17:27 |
| bac | do we actually use that anywhere? | 17:28 |
| henninge | once | 17:28 |
| henninge | bac: It's about imports that need other imports. | 17:28 |
| henninge | I feared we'd have more problems like that but luckily only two. | 17:29 |
| henninge | one of which is lib/canonical/launchpad/interfaces/__init__.py which is the only use of the "SKIP" comment. | 17:29 |
| henninge | the other one is in lp.code | 17:30 |
| henninge | bac: sorry, "imports that need other imports" is badly put. It's about import ordering. | 17:30 |
| bac | henninge: so there are imports that must be done in a specific order or they fail? | 17:31 |
| henninge | bac: yes, or so says the comment. | 17:31 |
| henninge | I don't like that very much, either. | 17:31 |
| henninge | let me find it | 17:31 |
| bac | cool | 17:31 |
| henninge | lib/lp/code/bzr.py | 17:32 |
| === benji is now known as benji-lunch | ||
| === matsubara is now known as matsubara-lunch | ||
| henninge | bac: I will end my week now. Do you have any more questions? | 17:47 |
| bac | henninge: no. | 17:50 |
| henninge | cool | 17:50 |
| bac | henninge: sadly, i just ran your tool against lp/registry and it created a 231 line diff | 17:50 |
| henninge | oops | 17:51 |
| bac | so in just a week we've diverged that much | 17:51 |
| bac | henninge: one more question | 17:54 |
| bac | henninge: you say embedded comments will break the formatter. is that reported at all or is it so broken you don't get a chance? | 17:55 |
| henninge | bac: it's not reported at all | 17:58 |
| bac | henninge: ok. i guess it'll create such a mess that it'll be easily discovered | 17:59 |
| henninge | bac: we only had one case like that in the tree that I simply fixed before running the tool | 17:59 |
| bac | sorry to keep you henninge. i'll finish the review after lunch. have a good weekend. | 17:59 |
| henninge | bac: thank you | 17:59 |
| henninge | bac: enjoy your lunch ;-) | 18:00 |
| bac | henninge: i shall...but it's getting cold. | 18:00 |
| henninge | sorry to keep you ... ;) | 18:00 |
| === Ursinha-lunch is now known as Ursinha | ||
| === salgado-lunch is now known as salgado | ||
| === benji-lunch is now known as benji | ||
| === matsubara-lunch is now known as matsubara | ||
| === bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| benji | indeed; I had almost exactly the same reaction | 19:47 |
| benji | the thing is, the few things I'd like (probably the same ones you're referring to) would be pretty easy to implement in Python 2 | 19:47 |
| benji | I really wish I had the desire to be the new BDFL. Now's the time for a Coup d'état. | 19:48 |
| benji | yeah, I expect most of those won't, but I don't see why most of those couldn't be backported | 19:49 |
| benji | oops, I'm in the wrong chan | 19:50 |
| benji | bac: watch out! https://code.launchpad.net/~benji/launchpad/bug-622765/+merge/33951 | 20:04 |
| * bac ducks | 20:06 | |
| benji | heh | 20:06 |
| === benji changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| benji | bac: for some reason my MP just shows "Approve (code)" and still shows "Needs review" at the top. That's the third time that's happened to me. Has the review UI changed for reviewers? | 20:19 |
| * benji really needs to start on getting his reviewer merit badge. | 20:20 | |
| bac | benji: that reason would be it is a two step process and i forgot to do the second step | 20:20 |
| benji | it's the processes fault ;) | 20:20 |
| bac | benji: reviewers should set it. when they forget, you can still proceed using 'ec2 land --force' ... but it is better to just bug them. | 20:21 |
| bac | benji: done | 20:21 |
| benji | I like to bug people... everyone wins! | 20:22 |
| === benji changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jcsackett changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | hello, bac. | 21:17 |
| jcsackett | https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-official_codehosting/+merge/33953 | 21:17 |
| jcsackett | have time for another review today? | 21:17 |
| bac | jcsackett: sure | 21:17 |
| jcsackett | awesome. | 21:17 |
| jcsackett | bac, this one shouldn't take too long. | 21:18 |
| jcsackett | apologies if it does. | 21:18 |
| === jcsackett is now known as jcsackett|afk | ||
| === jcsackett|afk is now known as jcsackett | ||
| === bac changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-afk | ||
| jcsackett | bac: re your comments, i actually thought it was clearer with the full if structure than "var = (usage_enum == ServiceUsage.something). you think the compressed version would be better? | 22:43 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!