Re: Refactor elpa/oauth2.el to use plist over cl-defstruct

[Xiyue Deng]

Soham Gumaste <sohamg2@gmail.com> writes:

> Hello,
>
> Thanks for taking the time to reply!
>
>> 
>> I think this will break clients using the current interface of oauth2.el
>> (including one of my small hack to use oauth2 for email authentication).
>> Is there any functionalities that are missing and requiring such a
>> breaking change?
>> 
>
> Yes, and that is my biggest concern too.
>
>> 
>> Agreed.  ISTR the recent discussion on cl-lib, which AIUI was about
>> preloaded ELisp.  The other majority of code is not bound by this
>> requirement, and cl-lib does make some tasks easier.  (But please do
>> correct me.)
>> 
>
> In this case, since `plstore` is used to store the information, it would 
> make it much easier to use a plist as it could be stored and retrieved 
> as-is, instead of having to convert between struct<-->plist every time.
>

Personally I don't see this serialization/deserialization to be a big
concern, and cl-defstruct makes accessing the fields easier.

>
>> 
>> Ditto.  Do you have a proof-of-concept patch to look at?
>> 
>
> I was writing the patch when I realised this change would make it 
> easier, which is why I asked. My idea is to store a (lambda) function as 
> the :secret attribute in the plstore, so that `auth-info-password` would 
> invoke it when the secret is requested, resulting in a refresh if 
> needed.

I think this was also what Andrew suggested.

> I also want to add other parameters to the plstore entry like 
> :host, :port and :user.
>

I think these information is better saved in the authinfo file, as those
are permanent information.  Currently the oauth2.plstore file only
stores ephemeral information, e.g. access token which expires and needs
to refresh, whereas the authinfo file store information that are always
valid.  In this arrangement, it's ok to remove oauth2.plstore completely
and regenerate all its data.  Personally I find this nice to work with
and it would be good to keep this separation of data storage.

> Secondly, another breaking change I want to propose is renaming the 
> default plstore file from `oauth.plstore` to `oauth.plist`, as the 
> auth-sources library uses the `.plist` extension to trigger the plstore 
> backend instead of the netrc backend (this is not documented directly, I 
> read the source code).
>

This could also be a breaking change as people may have been using the
old name already.  It's better to check whether the old file exists
before defaulting to the new name.

> I will report back in this thread with a proof of concept soon, with the 
> current cl-defstruct implementation. We can discuss further based on that.
>

Looking forward to it!

> Thanks

-- 
Regards,
Xiyue Deng

signature.asc
Description: PGP signature