noib3/nvim-oxi

Use well known TryFrom instead of custom FromObject / ToObjet tratis

Opened this issue · 4 comments

Maybe it's just me overlooking something but both traits in nvim_types::conversion are just a specialized version of TryFrom/TryInto.
Is there a technical reason?
Else I suggest to rewrite conversion.rs to use TryFrom instead or delete it all together.

I'm currently exploring nvim-oxi and while toying around with it I wrote a bunch of TryFrom<Object> implementations before I realized FromObject is a thing, if it's down to development time needed I could do the conversion and create a PR.

noib3 commented

It's mostly a matter of ergonomics. If we were to use TryFrom most functions in the nvim_oxi::api module would go from

use nvim_oxi::api;

pub fn get_option<Opt>(name: &str) -> Result<Opt, api::Error>
where
    Opt: FromObject, 
{
    ..
}

to

use nvim_oxi::api;

pub fn get_option<Err, Opt>(name: &str) -> Result<Opt, api::Error>
where
    Opt: TryFrom<Object, Error = Err>,
    Err: std::error::Error + Into<api::Error>,
{
    ..
}

and we'd also need to add a way to turn any std::error::Error into a generic variant of api::Error.

It also becomes more verbose when you're calling the function because you have an extra generic:

api::get_option::<bool>("modified") ->  api::get_option::<MyError, bool>("modified")

Tbh I don't think it's worth it.

I don't think adding a new generic is necessary a quick test shows

pub fn get_option<Opt>(name: &str) -> Result<Opt>
where
    Opt: TryFrom<Object>,
    Error: From<Opt::Error>,
{}

should work (it checks, i haven't tested it extensively)

Also I think the ergonomics improve quite a bit for users of the library.

But ultimately it's up to you. I'd be happy to implement it if you consider merging it.

noib3 commented

I've been experimenting with this on a new branch and I believe both of my concerns can be resolved.

Like you pointed out we don't actually need to spell out the error type associated w/ the TryFrom implementation, and we can introduce a new ToApiError trait together with a blanket impl to automatically convert any StdError into a nvim_oxi::api::Error.

This is a first rough sketch of this. If you want to work on the rest I'd consider merging a PR.

I've hit a bit of a showstopper for this I think, my attempt at converting this over either has a conflicting

impl<T> TryFrom<Object> for Option<T>

because of the

impl<T, U> TryFrom<U> for T where U: Into<T>;

in core or does not properly convert values which might be null in neovim to an Option<T>.