[00:54] <huwshimi> Hi, is anyone available to review a CSS change? https://code.launchpad.net/~huwshimi/launchpad/highlight-links-367877/+merge/45947
[12:03] <jcsackett> jtv: got room for another in your queue? https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938
[12:20] <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
[15:40] <benji> EdwinGrubbs: are you reviewing today?
[15:48] <EdwinGrubbs> benji: yes
[15:49] <jtv> hi Edwin :)
[15:49] <EdwinGrubbs> jtv: hi
[16:00] <benji> k
[16:02] <jtv> Whoa getting crowded here.  Lemme just fix that.
[16:09] <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:10] <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:11] <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:12] <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:13] <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:14] <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:15] <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:16] <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:17] <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:18] <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:19] <leonardr> sinzui: ok, let me know if you need anything (such as me re-understanding how lazr.restful uses snapshots)
[16:20] <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:21] <jcsackett> er, incompatibility, not error.
[16:27] <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:28] <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:41] <jcsackett> no problem.
[19:21] <EdwinGrubbs> benji: I have to leave for an appointment, but I'll be back later. Fortunately, the queue is empty.
[19:22] <benji> ok
[19:47] <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.
[20:03] <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:44] <thumper> https://code.launchpad.net/~thumper/launchpad/view-invalid-recipes/+merge/46037
[20:44] <thumper> hello reviewers :)
[21:02] <thumper> benji: have you got time to look at my review?
[21:02] <benji> thumper: sure
[21:02] <thumper> benji: ta
[21:15] <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:16] <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:20] <benji> thanks
[23:09] <EdwinGrubbs> thumper: why does getViewBrowser() log in as anonymous?
[23:10] <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:11] <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:12] <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:13] <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