| huwshimi | Hi, is anyone available to review a CSS change? https://code.launchpad.net/~huwshimi/launchpad/highlight-links-367877/+merge/45947 | 00:54 |
|---|---|---|
| === jtv-afk changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jtv-afk is now known as jtv | ||
| === matsubara-afk is now known as matsubara | ||
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: Ursinha || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | jtv: got room for another in your queue? https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938 | 12:03 |
| jcsackett | jtv, i think i pinged you on this when you had dropped from the channel. there room in the queue for one more? http://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938 | 12:20 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: jtv || reviewing: Ursinha || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === henninge_ is now known as henninge | ||
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: jcsackett || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === gary_poster is now known as gary-afk | ||
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| benji | EdwinGrubbs: are you reviewing today? | 15:40 |
| EdwinGrubbs | benji: yes | 15:48 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jtv, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | hi Edwin :) | 15:49 |
| EdwinGrubbs | jtv: hi | 15:49 |
| === gary-afk is now known as gary_poster | ||
| benji | k | 16:00 |
| === benji changed the topic of #launchpad-reviews to: On call: jtv, Edwin, benji || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === deryck is now known as deryck[lunch] | ||
| jtv | Whoa getting crowded here. Lemme just fix that. | 16:02 |
| === jtv changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | jtv: i've replied to your comment on my review. the only question i had was what were you foreseeing where it breaks API? | 16:09 |
| jcsackett | snapshotting of the objects are still allowed, but those fields won't be snapshotted when the entire object is. the collections themselves can be snapshotted independently. | 16:10 |
| jtv | I was just looking at it. | 16:10 |
| jtv | I'm just wondering whether any clients might erroneously be expecting to get the whole collection at once. | 16:10 |
| jtv | I may be wrong, but I assumed that collection snapshots aren't batched. | 16:11 |
| jcsackett | jtv: hm. so if a client tries to snapshot a product expecting those fields to be in the snapshot? | 16:11 |
| jcsackett | jtv: if you do like "thing = product.milestones; Snapshot(thing)" you shouldn't have any problem. | 16:11 |
| jtv | (BTW I just ran tests with the setUp methods removed and got the same results as with them still in) | 16:12 |
| jcsackett | jtv: that's good news--that didn't used to be the case. i'll kill that then. | 16:12 |
| jtv | Once you have a setUp, you do need to call super. But if you don't, you're good. | 16:12 |
| jtv | Anyway, about the snapshots: I guess the real question is whether the compatibility is defined at the launchpadlib API level (or equivalent), or "on the wire." | 16:13 |
| jcsackett | jtv: hm. i dunno. i do know that in the error case this is patching the API will explode anyway. | 16:14 |
| jtv | Actually, I wouldn't be at all surprised if sinzui had dealt with this question before. | 16:14 |
| jtv | True, incompatibility ain't so bad if something goes from nonfunctional to functional. | 16:15 |
| sinzui | The snapshot code does work over the api | 16:15 |
| sinzui | and you certainly cannot call launchpad_project.lp_save() at the moment | 16:16 |
| sinzui | I have tried several times over the past few weeks | 16:16 |
| jcsackett | it's blowing up with the same exception, isn't it? | 16:16 |
| sinzui | yep | 16:16 |
| leonardr | sinzui: what's going on here? is it something i should look at? | 16:17 |
| sinzui | leonardr: lazr.lifecycle.snapshot does a deep copy of objects we do not want to copy | 16:17 |
| jcsackett | leonardr: it's a shortlist error i've got a branch in review to fix. jtv has raised the question that the branch may break API compatability, as some fields are now marked doNotSnapshot. | 16:17 |
| jtv | (with "may" meaning "I don't know" ☺ ) | 16:18 |
| jcsackett | jtv: right. :-) | 16:18 |
| sinzui | leonardr: API is fine. We have logic to ensure we do not do something stupid. The logic is telling us we did do something stupid | 16:18 |
| leonardr | sinzui: ok, let me know if you need anything (such as me re-understanding how lazr.restful uses snapshots) | 16:19 |
| jcsackett | so, anyway, i'm probably not understanding the worry, but i don't think this breaks API compatibility. i think it fixes an issue that has hit the API too. | 16:20 |
| jcsackett | but i'm about as certain of that as i think you are that it does cause an error, jtv. which is to say: not certain at all. :-P | 16:20 |
| jcsackett | er, incompatibility, not error. | 16:21 |
| jcsackett | jtv: i'll fix the other problems and pursue the incompatibility question. thanks. | 16:27 |
| jtv | jcsackett: great, thanks. I'm close to EOD, so don't be shy to approach someone else if you want a final stamp of approval. | 16:27 |
| jcsackett | jtv: will do. thanks, and have a good evening. :-) | 16:28 |
| jtv | Thanks. Sorry I couldn't contribute more on the compatibility question than to ask it. | 16:28 |
| jcsackett | no problem. | 16:41 |
| === deryck[lunch] is now known as deryck | ||
| === benji is now known as benji-lunch | ||
| === benji-lunch is now known as benji | ||
| === gary_poster is now known as gary-lunch | ||
| EdwinGrubbs | benji: I have to leave for an appointment, but I'll be back later. Fortunately, the queue is empty. | 19:21 |
| === EdwinGrubbs is now known as Edwin-afk2 | ||
| benji | ok | 19:22 |
| === matsubara is now known as matsubara-afk | ||
| === gary-lunch is now known as gary_lunch | ||
| === gary_lunch is now known as gary_poster | ||
| jcsackett | benji: can you take a look at the revised diff on https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938? | 19:47 |
| benji | jcsackett: sure | 19:47 |
| jcsackett | thanks. | 19:47 |
| benji | jcsackett: Approved* with a couple of tiny tweaks. (*I'll ask Edwin to review my review when he gets back.) | 20:03 |
| jcsackett | benji: thanks. | 20:03 |
| benji | np | 20:03 |
| thumper | https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037 | 20:44 |
| thumper | hello reviewers :) | 20:44 |
| thumper | benji: have you got time to look at my review? | 21:02 |
| benji | thumper: sure | 21:02 |
| thumper | benji: ta | 21:02 |
| === benji changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: thumper || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Edwin-afk2 is now known as EdwinGrubbs | ||
| benji | thumper: Approved (but since I'm mentoring, Edwin will have to review my review) | 21:15 |
| thumper | benji: ack | 21:15 |
| thumper | thansk | 21:15 |
| benji | np | 21:16 |
| benji | EdwinGrubbs: I've done two small reviews since you were gone: https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037 and the revised diff on https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938 | 21:16 |
| EdwinGrubbs | I'll look at them | 21:16 |
| benji | thanks | 21:20 |
| === benji changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === benji changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | thumper: why does getViewBrowser() log in as anonymous? | 23:09 |
| thumper | EdwinGrubbs: it doesn't it logs in as a new user | 23:10 |
| thumper | it logs in as anon iff you specify no login | 23:10 |
| * thumper thinks | 23:10 | |
| EdwinGrubbs | thumper: there is a login(ANONYMOUS) call in that function. | 23:11 |
| EdwinGrubbs | that isn't in the if-statement | 23:11 |
| thumper | EdwinGrubbs: where exactly, I'll look | 23:11 |
| thumper | ah... | 23:11 |
| thumper | in the canonical url generation? | 23:11 |
| EdwinGrubbs | oh | 23:11 |
| thumper | I think it is to get it into a known login state | 23:12 |
| thumper | as login effectively logs out anyone else | 23:12 |
| thumper | and an interaction is needed for canonical url generation | 23:12 |
| thumper | and since we don't want to specify a particular user | 23:12 |
| EdwinGrubbs | thumper: ok, can you add a comment for other confused souls? Also, can you make a drive-by cleanup of ",rootsite=" in that method? | 23:12 |
| thumper | as it adds confusion... | 23:12 |
| StevenK | Yes, so do everything *first* and then call .getViewBrowser | 23:12 |
| thumper | ack | 23:13 |
| thumper | EdwinGrubbs: yes, I can do that | 23:13 |
| StevenK | See first test in the diff at https://code.launchpad.net/~stevenk/launchpad/bpb-currentcomponent-assertion-part-2/+merge/45693 | 23:13 |
| EdwinGrubbs | I'll update the mp after launchpad is back up | 23:13 |
| mwhudson | maybe that method should use restoreInteraction | 23:13 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!