Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use well known TryFrom instead of custom FromObject / ToObjet tratis #74

Open
cafce25 opened this issue Oct 19, 2022 · 4 comments
Open

Comments

@cafce25
Copy link

cafce25 commented Oct 19, 2022

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.

@cafce25 cafce25 changed the title Use well known TryFrom instead of custom tratis Use well known TryFrom instead of custom FromObject / ToObjet tratis Oct 19, 2022
@noib3
Copy link
Owner

noib3 commented Oct 19, 2022

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.

@cafce25
Copy link
Author

cafce25 commented Oct 19, 2022

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
Copy link
Owner

noib3 commented Oct 19, 2022

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.

@cafce25
Copy link
Author

cafce25 commented Apr 14, 2023

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>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants