Odd location for UnMarshal

[endophage]

First, great work, just came across this in the golang weekly newsletter and I've felt your pain re. authenticating with other services.

Obviously down to individual developer's opinions but it feels like Provider.UnmarshalSession should really be an initialization function for Session objects in the provider's session.go, not least of all because its symmetrical partner, Session.Marshal is on the Session. UnmarshalSession makes no use of the Provider instance it is passed so at the very least (and it's a non-breaking change), it shouldn't be a Provider pointer but a Provider object in the signature, to make it clear the function doesn't modify the Provider instance it is called on.

I'm guessing you've put it on the Provider so you can include it in the interface and enforce the interface for other providers people write. In that case, I guess I'm saying it seems to make more sense on the Session if it has to be an instance method.

[markbates]

Open a PR with the changes you're suggesting if it makes sense I'll merge it in.

[rakesh-eltropy]

Moved UnmarshalSession method in Session.

[zmb3]

I think the intent of the issue was not that we need to cut and paste the UnmarshalSession method into a different file, but that the method should not be on the Provider type.

As mentioned, the UnmarshalSession method doesn't even use it's *Provider receiver, and it doesn't make sense to have Marshal in the Session interface and unmarshal in the Provider type.

[rakesh-eltropy]

Sorry, It was a mistake. I rollbacked it.

[bentranter]

Closing this due to age, feel free to re-open if it's still an issue.