| jtv | Anyone up for a review? https://code.edge.launchpad.net/~jtv/launchpad/recife-review/+merge/33183 | 05:52 |
|---|---|---|
| jtv | thumper, you perhaps? ^^ | 06:14 |
| * thumper isn't here | 06:14 | |
| thumper | gah | 06:14 |
| * thumper waves hand in front of jtv | 06:16 | |
| thumper | thumper is not the reviewer you are looking for | 06:16 |
| jtv | thumper: then who is? | 06:17 |
| jtv | I bugged mwhudson last time, poolie isn't here… | 06:17 |
| thumper | jtv: my guess would be stub is the closest to you | 06:17 |
| jtv | I don't see him here. | 06:18 |
| thumper | jtv: what does get_traits(template) do? | 06:22 |
| jtv | It retrieves the TranslationSideTraits for the TranslationSide that template is on. | 06:23 |
| jtv | TranslationSideTraits is a traits object to describe e.g. "if I'm translating on the Ubuntu side, what's the flag I should set to make a message the current one for that side?" | 06:24 |
| jtv | And "how do I query for the message that's current on this side?" | 06:24 |
| jtv | A TranslationMessage has two flags: "this is the current message on the Ubuntu side" and "this is the current message on the upstream side." | 06:25 |
| thumper | jtv: do you know about assertSqlAttributeEqualsDate | 06:25 |
| thumper | ? | 06:25 |
| jtv | No | 06:25 |
| thumper | you could use... | 06:26 |
| jtv | That's a thought. I'd also have to inject the right date though. | 06:26 |
| thumper | self.assertSqlAttributeEqualsDate( | 06:26 |
| thumper | current, 'date_reviewed', UTC_NOW) | 06:26 |
| thumper | which is what I think you really want to check | 06:27 |
| jtv | But it doesn't have to be UTC_NOW. | 06:27 |
| jtv | It could be the python-side clock from a few ticks ago. | 06:28 |
| thumper | but you haven't committed | 06:28 |
| thumper | so it will be the UTC_NOW constant | 06:29 |
| thumper | see lib/lp/code/model/tests/test_branch.py:152 | 06:29 |
| * thumper laughs | 06:30 | |
| thumper | the only tests that use this assertion method are lp.code tests | 06:30 |
| thumper | :) | 06:30 |
| jtv | It involves changing markReviewed, but UTC_NOW does make more sense. | 06:32 |
| thumper | what changes in markReviewed? | 06:32 |
| jtv | It currently uses the python clock. | 06:32 |
| thumper | ew | 06:33 |
| jtv | I didn't think that UTC_NOW got deferred until commit though; doesn't it get set when the object is flushed to the store? | 06:33 |
| thumper | it is the transaction start time | 06:33 |
| thumper | IIRC | 06:33 |
| thumper | but yes | 06:33 |
| thumper | the setting happens when flushed | 06:34 |
| thumper | but the assertion method checks for UTC_NOW | 06:34 |
| thumper | we tend to use timestamp args like | 06:34 |
| thumper | _date_requested=None | 06:35 |
| thumper | if _date_requested is None: | 06:35 |
| thumper | _date_requested = UTC_NOW | 06:35 |
| jtv | Yes, it's the transaction start time. But AIUI it's a special constant when you set it, and then when the object gets flushed or invalidated, it gets replaced with an actual timestamp. | 06:35 |
| thumper | that way tests can ovverride | 06:36 |
| thumper | jtv: that's my understanding too | 06:36 |
| jtv | So have to be careful with unexpected flushing. | 06:36 |
| thumper | we have the params start with an underscore to indicate that they are private params | 06:36 |
| jtv | What is a private param? | 06:36 |
| jtv | It seems a contradiction in terms. | 06:36 |
| thumper | :) | 06:36 |
| thumper | one whose default value is used almost all of the time | 06:37 |
| thumper | but other internal model code may pass something | 06:37 |
| thumper | and tests often pass it | 06:37 |
| thumper | def requestReview(self, _date_requested=None): | 06:37 |
| thumper | no browser or external code should pass the param | 06:37 |
| thumper | but internal or tests are OK | 06:37 |
| thumper | that is what we mean by private param | 06:37 |
| jtv | Of course another thing I could do is just override current.markReviewed and verify that it gets called. :) | 06:38 |
| jtv | current.markReviewed = FakeMethod() | 06:38 |
| jtv | self.assertEqual(1, current.markReviewed.call_count) | 06:38 |
| thumper | if that is all you are wanting to test, then yeah, sure | 06:38 |
| jtv | It may be a bit short on integration-testing. | 06:39 |
| jtv | I'm going with UTC_NOW and assertSqlAttributeEqualsDate. | 06:42 |
| thumper | ok | 06:42 |
| jtv | It really is better that way. I wonder why I didn't find that method—isn't it in lib/lp/testing/__init__.py? | 06:43 |
| jtv | thumper: pushed that change already btw so you may want to reload the diff | 06:51 |
| thumper | jtv: why did you not just replace datetime.now(UTC) with UTC_NOW instead of changing the default param? | 06:52 |
| thumper | jtv: it is more idiomatic to have the default be None | 06:53 |
| jtv | thumper: I just didn't see the point in having the extra step. | 06:53 |
| jtv | I think the None is mainly more idiomatic for cases like [] and {} where things go wrong if you specify the default you really want. | 06:54 |
| thumper | and UTC is a constant? | 06:54 |
| jtv | Although on the other hand there's the pain of specifying the same default in the interface. | 06:54 |
| thumper | true | 06:54 |
| thumper | go for None I reckon | 06:55 |
| jtv | OK | 06:55 |
| jtv | It's pushed, but still no diff update | 06:59 |
| thumper | done | 07:01 |
| jtv | Thanks. | 07:01 |
| jtv | lifeless, you may have some useful thoughts on something I want to do. | 07:07 |
| jtv | guten Morgen henninge | 07:11 |
| henninge | Goedemorgen jtv! ;) | 07:15 |
| * henninge has to remember that w/o looking it up. | 07:16 | |
| henninge | jtv: สวัสดี | 07:16 |
| jtv | สวัสดีครับ | 07:16 |
| jtv | ສະບາຍດີ | 07:17 |
| === 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 | ||
| lifeless | jtv: shoot | 09:34 |
| jtv | lifeless: hi! It's about the fake librarian. | 09:34 |
| jtv | It catches transaction.commit() in order to simulate the librarian's transactional behaviour. | 09:35 |
| jtv | (Except of course that it gives something clearer than LookupError if you haven't committed your file yet) | 09:35 |
| jtv | however | 09:35 |
| jtv | I would like to allow tests to avoid the commit altogether. | 09:36 |
| jtv | So many tests commit just to satisfy the librarian. With the fake librarian, we could avoid those commits. | 09:36 |
| jtv | Now, I can't in good faith add a method to the Librarian API. | 09:37 |
| jtv | So where do I put this? | 09:37 |
| lifeless | why can't you ? | 09:37 |
| lifeless | it might be better to let tests commit | 09:37 |
| lifeless | and stub out the whole db | 09:38 |
| lifeless | bzr tests do many very expensive commits and are still much faster than lp commits | 09:38 |
| lifeless | s/lp commits/lp tests/ | 09:38 |
| lifeless | anyhow; I don't understand why you can't make the librarian api better in tests | 09:39 |
| jtv | Well the call I have in mind would basically say "pretend there's been a commit as far as the librarian is concerned." | 09:39 |
| jtv | That's not even something we _can_ pretend with the real librarian. | 09:39 |
| lifeless | so | 09:41 |
| lifeless | I don't understand the problem | 09:41 |
| jtv | That's because I haven't really gotten around to formulating it properly yet. | 09:41 |
| jtv | The problem at hand is: I want to allow a test, | 09:42 |
| jtv | which uses the fake librarian, | 09:42 |
| jtv | to tell the fake librarian: "give up the pretense that you don't know about these files just because I haven't committed yet; I don't _want_ to commit." | 09:42 |
| jtv | This is something you'd do after creating test data, before the operation you're going to test. | 09:43 |
| lifeless | ok | 09:43 |
| lifeless | so do that? | 09:43 |
| jtv | Quite. | 09:43 |
| jtv | As long as it's only a pretense for the librarian's benefit, I feel I can't have a method for that in the librarian API—but that makes it hard to invoke on the utility. | 09:44 |
| lifeless | again, I don't understand. | 09:44 |
| lifeless | perhaps it would help if I describe how I'd do it. | 09:45 |
| jtv | ga | 09:45 |
| lifeless | I'd create an interface FakedLibrarian | 09:45 |
| jtv | (ga the acronym, not the onomatopeia) | 09:45 |
| lifeless | extending Librarian | 09:45 |
| lifeless | (pseudonyms, I know the names are different) | 09:45 |
| jtv | There's an ILibrarianClient. | 09:45 |
| lifeless | in FL I'd add 'pretend_commit()' | 09:45 |
| lifeless | and in the test suite, the same utility is the fake librarian instance, which implements FL, and so it all works. | 09:46 |
| jtv | So I'd unregister the existing ILibrarianClient utility and register an IFakeLibrarianClient in its stead? | 09:46 |
| jtv | Would getUtility(ILibrarianClient) then let me access IFakeLibrarianClient methods?? | 09:49 |
| lifeless | yes | 09:49 |
| lifeless | because FLC extends LC it still meets the interface and can be a valid utility | 09:49 |
| lifeless | if necessary, you could just do IFLC(getU(LC)) | 09:51 |
| jtv | There are other alternatives, I suppose: let the test keep the fake librarian in self.fake_librarian and do self.fake_librarian.commit() | 09:52 |
| lifeless | sure | 09:52 |
| lifeless | either would seem to fit the zope style | 09:53 |
| jtv | For now, where a test sets it up manually, self.fake_librarian would be an unproxied object, so there's no need even to extend the interface. | 09:54 |
| jtv | Doesn't work so well if we turn it into a test resource or whatever, I suppose. | 09:54 |
| lifeless | btw | 09:54 |
| lifeless | lp:python-fixtures | 09:54 |
| lifeless | have a play, you might like it | 09:54 |
| jtv | lifeless: thanks. When there's time… 2011 is a bit full already but 2012 is looking good. | 10:02 |
| lifeless | jtv: its a factored out core component from testresources | 10:03 |
| jtv | Ah | 10:03 |
| lifeless | jtv: its a closer fit in some ways to layers and easier to work with | 10:03 |
| lifeless | I suggest it not to give you work, but to liberate you :) | 10:03 |
| jtv | :-) | 10:03 |
| wgrant | lifeless: What was the motive for splitting it out of testresources? | 10:05 |
| wgrant | (ie. why wouldn't one want to use testresources?) | 10:05 |
| lifeless | wgrant: separate concepts | 10:05 |
| lifeless | many places and projects to use conceptual fixtures exist separate from 'optimise hi cost setup/teardowns' | 10:06 |
| lifeless | wgrant: so this aims to be the one-true api for the fixture bit, and optimising layers on top | 10:06 |
| lifeless | wgrant: also, write one to throw away | 10:09 |
| jelmer | hi Abel | 11:26 |
| jelmer | adeuring1: Can I add a branch to the mp queue? | 11:27 |
| adeuring1 | jelmer: sure, I'll look at it | 11:27 |
| jelmer | adeuring1: Thanks. The MP is at https://code.edge.launchpad.net/~jelmer/launchpad/617393-cleanup-tmp/+merge/33194 | 11:27 |
| === adeuring1 is now known as adeuring | ||
| === 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 | ||
| adeuring | jelmer: in unpack_source(): output, unused = dpkg_source.communicate() -- is stderr really always unused? Or should possible stderr data be added to the data of DpkgSourceError? | 12:25 |
| jelmer | adeuring: The useful errors seem to end up on stdout; I'm not sure whether stderr is actually unused. | 12:29 |
| adeuring | jelmer: ok; it occured to me meanwhile that you didn't chnage that code anyway -- just moved it (sorry, was distracted from reviewing...) | 12:30 |
| adeuring | jelmer: In lines 447..453 of th diff: I think you should use assertRaises() instead of try/except | 12:44 |
| adeuring | otherwise, you don't check that the exception is indeed raised | 12:44 |
| jelmer | adeuring: I need to check that the exception has the right message in it | 12:45 |
| jelmer | adeuring: The self.fail() would be called in that case. | 12:46 |
| adeuring | jelmer: ah, right, I missed the fail() call. sorry | 12:47 |
| adeuring | jelmer: r=me | 12:50 |
| === 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 | ||
| jml | jelmer, you can use assertRaises for that! | 12:58 |
| jml | jelmer, e = self.assertRaises(ThingyException, f, blah, blah); self.assertEqual("Mishandled thingy", str(e)) | 12:59 |
| jelmer | adeuring: Thanks! | 13:11 |
| jelmer | jml: Ah, I wasn't aware assertRaises returned the exception. Thanks | 13:11 |
| === mrevell is now known as mrevell-lunch | ||
| === 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 | ||
| bac | hello adeuring | 13:34 |
| adeuring | hey bac! | 13:42 |
| bac | adeuring: how goes the battle? | 13:42 |
| adeuring | bac: I was distracted by a few other issues -- did just in review... | 13:42 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === mrevell-lunch is now known as mrevell | ||
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: mbp,allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === salgado changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: mbp,allenap || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| salgado | adeuring, bac, I've just added a long and boring branch to your queue; hope you'll forgive me | 14:31 |
| bac | salgado: np | 14:32 |
| adeuring | salgado: any bribe offers ? ;) | 14:32 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: mbp,- || queue: [salgado] || 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: salgado,- || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| salgado | adeuring, a promise that the next one will not be boring as this. does that count as a bribe? ;) | 14:50 |
| adeuring | salgado: sure :) | 14:50 |
| gmb | adeuring, Can I add https://code.edge.launchpad.net/~gmb/launchpad/new-subscriptions-widget-bug-616653/+merge/33215 to your queue (it's the basic subscription widget work) | 14:54 |
| adeuring | gmb: go ahead | 14:55 |
| === gmb changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado,- || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | adeuring, Thanks. Didn't notce that bac was OCR too. Hi bac. | 14:55 |
| bac | morning gmb | 14:56 |
| bac | gmb i'll take your branch. | 14:56 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado, gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | bac, Thanks. It's a bit of a weird one because it feels like it doesn't add any value, but it's a get-the-ball-rolling branch rather than actually adding any functionality just yet. | 14:57 |
| bac | gmb: ok. using the salgado/sinzui cadence fu? | 14:58 |
| gmb | bac, Yes. Well, I'm also using the "I'll kill myself if I don't land *something*" cadence. | 14:59 |
| bac | gmb, it's august fer crying out loud! get on your 2010! | 15:03 |
| gmb | bac, Aaaaaaagh | 15:03 |
| gmb | CPE. | 15:03 |
| bac | :) | 15:03 |
| bac | gmb: can't we just get one of the script gurus to fix up all copyrights statements on their xmas break for the coming year? | 15:04 |
| gmb | bac, Yes, lets :) | 15:04 |
| bac | gmb: I think 'A change is made to the bug (do not notify me about new comments).' would be better as 'A change is made to the bug, but do not notify me about new comments.' What do you think? | 15:07 |
| bac | gmb: hmm, is that comma extraneous? | 15:07 |
| gmb | bac, I had the same problem when trying to work out what that label should read :) | 15:09 |
| gmb | A semicolon would replace the ', but' appropriately, I think. | 15:09 |
| bac | gmb: classic bikeshedding. i just think avoiding the parenthetical aside works | 15:09 |
| gmb | bac, 'A change is made to the bug; do not notifiy me about new comments' reads better, I think. | 15:10 |
| gmb | Although without the spelling error, obviously. | 15:10 |
| bac | gmb: ok. hey, why don't we defer to our wordsmith, since we're supposed to anyway? (conjures mrevell) | 15:10 |
| * mrevell reads | 15:11 | |
| bac | gmb: that is a boatload of scaffolding just for a simple little test! | 15:11 |
| * gmb imagines mrevell in harem pants and curly shoes, with a Dali mustache. | 15:11 | |
| mrevell | hah | 15:11 |
| mrevell | semi-colons ftw | 15:12 |
| gmb | bac, I know. Actually, I'll try to trim some of it out; it's been a while since I did JS tests, so I've cargo-culted. | 15:12 |
| gmb | mrevell, Thanks. | 15:12 |
| mrevell | :) | 15:12 |
| bac | r=bradley | 15:13 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: salgado, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | bac, Thanks. | 15:15 |
| adeuring | salgado: r=me | 15:23 |
| salgado | thanks adeuring! | 15:25 |
| === 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 | ||
| === Ursinha is now known as Ursinha-lunch | ||
| EdwinGrubbs | adeuring, bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout/+merge/32555 | 16:42 |
| bac | EdwinGrubbs: i will | 16:42 |
| === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -, Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | bac: I'm going to drive to a coffee shop, so I'll be incomunicado for a few minutes. | 16:42 |
| bac | ok | 16:43 |
| === adeuring changed the topic of #launchpad-reviews to: On call: bac || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| allenap | bac: Thank you for reviewing my refactor-get-email-notifications branch earlier :) | 17:10 |
| bac | allenap: it's my job. :) nice branch | 17:12 |
| bac | allenap: what's up with all of these launchpad branches? you miss us? | 17:13 |
| allenap | bac: Yes :) | 17:13 |
| bac | well, come home soon | 17:13 |
| allenap | bac: Well, my rotation to Landscape is ending (early) in a couple of weeks, so not long. | 17:14 |
| bac | allenap: been fun? | 17:14 |
| allenap | bac: Yes, lots, but I am a bit home-sick :) | 17:15 |
| === deryck is now known as deryck[lunch] | ||
| EdwinGrubbs | bac: hmm, I'm not sure if my last message went through. I'm available for comments now. | 17:39 |
| bac | EdwinGrubbs: i've already approved it | 17:40 |
| bac | EdwinGrubbs: i did have a question about whether you can use the slave store | 17:40 |
| EdwinGrubbs | thanks | 17:40 |
| * bac reconsiders the phrase "slave store". sounds like something you'd see in charleston | 17:41 | |
| EdwinGrubbs | bac: oh, I answered that in the mp. sorry, I didn't see you asking that in the irc channel. | 18:14 |
| === Ursinha-lunch is now known as Ursinha | ||
| === 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 | ||
| bac | hi EdwinGrubbs, could you do a review for me? | 19:35 |
| EdwinGrubbs | bac: sure | 19:39 |
| bac | thanks EdwinGrubbs -- https://code.edge.launchpad.net/~bac/launchpad/bug-588773-charm/+merge/33248 | 19:40 |
| EdwinGrubbs | bac: oops, wrong channel | 20:00 |
| bac | hey EdwinGrubbs, over here :) | 20:00 |
| bac | though you might get a better answer in #canonical | 20:00 |
| bac | autoupdate? we don't know. | 20:01 |
| bac | there is a bug for its removal | 20:01 |
| EdwinGrubbs | who added it? | 20:01 |
| bac | we looked in the db and only one project has a value for it. | 20:01 |
| bac | EdwinGrubbs: it's been around a long time. | 20:01 |
| bac | 1ZRR16270358332506 | 20:02 |
| bac | ergh | 20:02 |
| bac | https://bugs.edge.launchpad.net/launchpad-registry/+bug/618926 | 20:02 |
| _mup_ | Bug #618926: Remove autoupdate from product model <tech-debt> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/618926> | 20:02 |
| lifeless | autoupdate | 20:02 |
| lifeless | we had this requirement to sync project metadata from freshmeat etc | 20:03 |
| EdwinGrubbs | I started to get excited when I thought you pasted your cc number, but alas, no free laptop for me. | 20:03 |
| lifeless | did 2 runs of the syncer or something and stopped. It was terrible ;) | 20:03 |
| lifeless | IIRC salgado knows all about it, but I'm pretty sure you can zap it | 20:03 |
| bac | EdwinGrubbs: no, much more boring. hey look, i got a delivery! | 20:04 |
| bac | thanks lifeless | 20:04 |
| bac | brb | 20:05 |
| EdwinGrubbs | bac: I get an oops if I try to change the person_standing or the personal_standing_reason on a person. | 20:37 |
| bac | EdwinGrubbs: ok, let me look | 20:37 |
| sinzui | I found a cron job that updates standing every day. It does nothing | 20:38 |
| sinzui | jcsackett, did you update your MP to needs review? | 20:43 |
| jcsackett | sinzui, i did; i was waiting for the diff to pop up before adding myself to the queue. | 20:43 |
| === sinzui changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| sinzui | jcsackett, undersood | 20:43 |
| jcsackett | and sinzui beats me into the queue. :-P | 20:44 |
| sinzui | I chose an arcane way to test that was the must less dangerous/evil | 20:44 |
| sinzui | jcsackett, look at https://code.edge.launchpad.net/~sinzui/launchpad/dsp-bug-counts-1/+merge/33257 | 20:45 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| sinzui | bac I have a branch for review (^), I think jcsackett should be ahead of me though if his is ready | 20:45 |
| jcsackett | bac: https://code.edge.launchpad.net/~jcsackett/launchpad/add-enums-to-models/+merge/33255 | 20:45 |
| bac | ok, i'll do them in that oder | 20:46 |
| bac | order | 20:46 |
| bac | odor | 20:46 |
| sinzui | jcsackett, I employed delegates and implements to make a test class. Those two helpers allow my class to be the type under test, and let me the code use the real object for non test parts. | 20:46 |
| jcsackett | sinzui: very nice. | 20:47 |
| jcsackett | that's really cool. | 20:47 |
| sinzui | jcsackett, We also use this trick to create helpers that construct data for views | 20:47 |
| bac | good catch, EdwinGrubbs. i failed to change permission in IHasStanding | 20:48 |
| === bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: jcs || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| lifeless | sinzui: hey, FYI | 20:52 |
| lifeless | sinzui: that milestone +index fix, I'm down to 46 failures | 20:52 |
| lifeless | all fallout from having to convert to storm | 20:52 |
| sinzui | wow | 20:52 |
| lifeless | sinzui: early next week I should have that conversion done; | 20:52 |
| lifeless | then the simple fix can actually land | 20:52 |
| lifeless | we could land an even simpler fix sooner if we wantd | 20:53 |
| lifeless | by precaching before we access any attributes on the bugs at all | 20:53 |
| lifeless | moving the list comprehension down a line | 20:53 |
| lifeless | sinzui: what do you think | 20:54 |
| sinzui | I have take similar fixes in the last few weeks. Getting some small improvement is still valuable to users | 20:55 |
| lifeless | ok | 20:55 |
| lifeless | so its the weekend now and we're moving house monday | 20:55 |
| lifeless | if I don't have the larger fix's issues actually resolved tuesday, I'll do the trivial thing. | 20:55 |
| sinzui | thanks lifeless. I will be about if you need a reviewer | 20:59 |
| lifeless | thanks | 21:01 |
| lifeless | won't be today | 21:01 |
| lifeless | off to buy a desk etc | 21:01 |
| EdwinGrubbs | bac: On the $projectgroup/+admin page, if you have launchpad.Moderate, the ProjectEditNavigationMenu shows that you are on the "Administer" page, but you don't see any other links, because you don't have launchpad.Edit. This looks especially bizarro, because there is a yellow edit-icon that you can't click on because you are already on that page. | 21:06 |
| bac | EdwinGrubbs: will investigate | 21:07 |
| EdwinGrubbs | sinzui: with regards to the ugly ui mentioned above, could the navigationmenu-related-pages.pt have a condition added, so that it doesn't display the menu if there is at least one clickable menu item? If that won't work, maybe we need some clearer indication that the link is disabled because you are already on this page and the edit-icon should appear in gray. | 21:09 |
| sinzui | We ran into this problem on the milestone +index page | 21:10 |
| sinzui | I thought the nav menu was updated to handle that case | 21:10 |
| sinzui | EdwinGrubbs, Your suggest is sound. I thought we had fixed that issue | 21:11 |
| EdwinGrubbs | bac: ^^^ | 21:12 |
| bac | jcsackett: it looks like your tests for distribution and product could be refactored and combined | 21:12 |
| jcsackett | bac: i thought about that, but wasn't sure where to stick the combined set. | 21:13 |
| bac | jcsackett: name the test for the functionality, not the pillar | 21:13 |
| bac | test_service_usage.py | 21:13 |
| jcsackett | bac: dig. | 21:13 |
| bac | put all of the goop into a base and the product and distribution tests will be teeny | 21:14 |
| bac | jcsackett: r=bac, with those suggested fixes | 21:15 |
| bac | jcsackett: make sure your base class doesn't execute too. | 21:15 |
| === bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: sinzui || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | bac: thanks. looking into it now. | 21:16 |
| === Ursinha is now known as Ursinha-bbl | ||
| bac | hi EdwinGrubbs. i've fixed the permission problem you found and updated the MP with an incremental diff | 22:22 |
| EdwinGrubbs | bac: r=me | 22:30 |
| bac | thanks EdwinGrubbs. have a nice weekend | 22:30 |
| bac | EdwinGrubbs: i think that UI fix is probably good and safe. just not for a friday evening | 22:31 |
| bac | sinzui: this does seem to work: http://pastebin.ubuntu.com/481143/ | 22:31 |
| sinzui | :( | 22:33 |
| sinzui | I think that looks right | 22:33 |
| EdwinGrubbs | bac: that's fine. there's no reason to tempt fate. | 22:34 |
| sinzui | bac: should we add the show_links to the render rule instead? | 22:35 |
| bac | sinzui: perhaps | 22:35 |
| sinzui | bac: navigationmenu-related-pages.pt? I think you want to hack global | 22:37 |
| sinzui | bac: navigationmenu-actions renders the action menu | 22:37 |
| sinzui | bac: related-pages is for that eye-sore that appears on some pages telling you that the page you are one may not have the complete information you are interested it | 22:38 |
| bac | sinzui: all good things to consider when we tackle this issue. mine was just a proof-of-concept since i'm not including it in my de-debacle branch | 22:39 |
| === 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 | ||
| bac | sinzui: did i mentiopn r=bac on your branch? | 22:40 |
| sinzui | faboo | 22:42 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!