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