[00:54] Hi, is anyone available to review a CSS change? https://code.launchpad.net/~huwshimi/launchpad/highlight-links-367877/+merge/45947 === 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 [12:03] jtv: got room for another in your queue? https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938 [12:20] 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 === 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 [15:40] EdwinGrubbs: are you reviewing today? [15:48] benji: yes === 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 [15:49] hi Edwin :) [15:49] jtv: hi === gary-afk is now known as gary_poster [16:00] k === 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] [16:02] Whoa getting crowded here. Lemme just fix that. === 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 [16:09] 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:10] 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] I was just looking at it. [16:10] I'm just wondering whether any clients might erroneously be expecting to get the whole collection at once. [16:11] I may be wrong, but I assumed that collection snapshots aren't batched. [16:11] jtv: hm. so if a client tries to snapshot a product expecting those fields to be in the snapshot? [16:11] jtv: if you do like "thing = product.milestones; Snapshot(thing)" you shouldn't have any problem. [16:12] (BTW I just ran tests with the setUp methods removed and got the same results as with them still in) [16:12] jtv: that's good news--that didn't used to be the case. i'll kill that then. [16:12] Once you have a setUp, you do need to call super. But if you don't, you're good. [16:13] 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:14] jtv: hm. i dunno. i do know that in the error case this is patching the API will explode anyway. [16:14] Actually, I wouldn't be at all surprised if sinzui had dealt with this question before. [16:15] True, incompatibility ain't so bad if something goes from nonfunctional to functional. [16:15] The snapshot code does work over the api [16:16] and you certainly cannot call launchpad_project.lp_save() at the moment [16:16] I have tried several times over the past few weeks [16:16] it's blowing up with the same exception, isn't it? [16:16] yep [16:17] sinzui: what's going on here? is it something i should look at? [16:17] leonardr: lazr.lifecycle.snapshot does a deep copy of objects we do not want to copy [16:17] 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:18] (with "may" meaning "I don't know" ☺ ) [16:18] jtv: right. :-) [16:18] 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:19] sinzui: ok, let me know if you need anything (such as me re-understanding how lazr.restful uses snapshots) [16:20] 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] 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:21] er, incompatibility, not error. [16:27] jtv: i'll fix the other problems and pursue the incompatibility question. thanks. [16:27] 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:28] jtv: will do. thanks, and have a good evening. :-) [16:28] Thanks. Sorry I couldn't contribute more on the compatibility question than to ask it. [16:41] no problem. === 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 [19:21] benji: I have to leave for an appointment, but I'll be back later. Fortunately, the queue is empty. === EdwinGrubbs is now known as Edwin-afk2 [19:22] ok === matsubara is now known as matsubara-afk === gary-lunch is now known as gary_lunch === gary_lunch is now known as gary_poster [19:47] benji: can you take a look at the revised diff on https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938? [19:47] jcsackett: sure [19:47] thanks. [20:03] jcsackett: Approved* with a couple of tiny tweaks. (*I'll ask Edwin to review my review when he gets back.) [20:03] benji: thanks. [20:03] np [20:44] https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037 [20:44] hello reviewers :) [21:02] benji: have you got time to look at my review? [21:02] thumper: sure [21:02] benji: ta === 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 [21:15] thumper: Approved (but since I'm mentoring, Edwin will have to review my review) [21:15] benji: ack [21:15] thansk [21:16] np [21:16] 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] I'll look at them [21:20] thanks === 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 [23:09] thumper: why does getViewBrowser() log in as anonymous? [23:10] EdwinGrubbs: it doesn't it logs in as a new user [23:10] it logs in as anon iff you specify no login [23:10] * thumper thinks [23:11] thumper: there is a login(ANONYMOUS) call in that function. [23:11] that isn't in the if-statement [23:11] EdwinGrubbs: where exactly, I'll look [23:11] ah... [23:11] in the canonical url generation? [23:11] oh [23:12] I think it is to get it into a known login state [23:12] as login effectively logs out anyone else [23:12] and an interaction is needed for canonical url generation [23:12] and since we don't want to specify a particular user [23:12] 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] as it adds confusion... [23:12] Yes, so do everything *first* and then call .getViewBrowser [23:13] ack [23:13] EdwinGrubbs: yes, I can do that [23:13] See first test in the diff at https://code.launchpad.net/~stevenk/launchpad/bpb-currentcomponent-assertion-part-2/+merge/45693 [23:13] I'll update the mp after launchpad is back up [23:13] maybe that method should use restoreInteraction