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

Improve the Error type #4

Merged
merged 3 commits into from
May 9, 2019
Merged

Conversation

koute
Copy link
Contributor

@koute koute commented May 8, 2019

So there are a few things I want to point out:

  1. Unfortunately the type_name intrinsic is still unstable, so by default the error messages will look like this:
cannot cast a &[u8] into a &[<Rust type>]: the slice's address is not divisible by the minimum alignment (2) of <Rust type> [turn on the \"nightly\" feature of the `byte-slice-cast` crate to see the actual type name]

and like this:

cannot cast a &[u8] into a &[<Rust type>]: the size (15) of the slice is not divisible by the size (2) of <Rust type> [turn on the \"nightly\" feature of the `byte-slice-cast` crate to see the actual type name]

I've added a disabled-by-default feature nightly which can be used by the user to enable the use of type_name and get the actual type name in the error message.

With the feature enabled those two error messages look like this:

cannot cast a &[u8] into a &[u16]: the slice's address is not divisible by the minimum alignment (2) of u16

and:

cannot cast a &[u8] into a &[u16]: the size (15) of the slice is not divisible by the size (2) of u16

Hopefully once type_name stabilizes we can just make this the default and make the nightly feature a no-op.

  1. I've renamed the error variants. This is very bikesheddy and I don't really have a good argument why I like them more this way (I just think they're nicer), so if you prefer the old names please do tell me and I'll be happy to revert.

@sdroege
Copy link
Owner

sdroege commented May 8, 2019

I'll review everything else later, but

Unfortunately the type_name intrinsic is still unstable, so by default the error messages will look like this:

I was thinking about this and one way around that would be to have associated string-typed constants in a trait that is implemented on the types we care about. And then either make the error type generic (hmm), or store the &'static str in there.

Not that nice but nicer than the error messages without nightly IMHO :)

@koute
Copy link
Contributor Author

koute commented May 8, 2019

I was thinking about this and one way around that would be to have associated string-typed constants in a trait that is implemented on the types we care about. And then either make the error type generic (hmm), or store the &'static str in there.

Not that nice but nicer than the error messages without nightly IMHO :)

In a way that works too, but that is also suboptimal in a way - eventually the type_name will get stabilized, and then there will be no point in requiring nor keeping that trait, so then we would probably want to remove it, but that will require us to break backwards compatibility again. On the other hand if we go with the current approach we will be able to make the switch without breaking anything.

(If only specialization was stable we could hide the trait as an implementation detail, but alas, that also in unstable.)

It's up to you though which approach you'd like to go for. (: (I'm fine either way as long as the traits will still be implementable outside the crate.)

@sdroege
Copy link
Owner

sdroege commented May 8, 2019

Do you know what the plan for stabilizing type_name is? Is it going to happen in the forseeable future or is it more like specialization and maybe at some point it will become stable but nobody knows? :)

@koute
Copy link
Contributor Author

koute commented May 8, 2019

Looking at the comments on the issue it's.... somewhat hard to tell. Your guess is probably as good as mine. It could probably be stabilized sooner (it's not a super complex feature) rather than later if someone could champion it, write a proper RFC and do all of the legwork, but there doesn't seem to be anyone like that right now.

@sdroege
Copy link
Owner

sdroege commented May 8, 2019

Then it seems ok to me to add a trait for this for now, and once it became stable we remove that trait again. It breaks backwards compatibility but will be very easy to update and probably won't affect most code out there at all.

@koute
Copy link
Contributor Author

koute commented May 8, 2019

Okay. I'll update the PR tomorrow.

@sdroege
Copy link
Owner

sdroege commented May 8, 2019

Great, thanks :)

@koute
Copy link
Contributor Author

koute commented May 9, 2019

Okay, you were right. It's definitely better to just use a trait, and since we can make it private it actually doesn't introduce any backward compatibility hazards.

I've also updated the changelog.

Please let me know if you'd like anything else changed.

@sdroege
Copy link
Owner

sdroege commented May 9, 2019

and since we can make it private

But if it's private, how would you make this crate work on custom types? I thought that was one of your requirements :)

@koute
Copy link
Contributor Author

koute commented May 9, 2019

and since we can make it private

But if it's private, how would you make this crate work on custom types? I thought that was one of your requirements :)

Yeah, that was a little brainfart on my part. (: As long as the Error enum's fields are public the user can just fill them out manually, so the trait doesn't actually need to be public.

@sdroege sdroege merged commit fc0c302 into sdroege:master May 9, 2019
@sdroege
Copy link
Owner

sdroege commented May 9, 2019

Indeed, I missed that little detail too. Nice :) Thanks a lot!

@sdroege
Copy link
Owner

sdroege commented May 9, 2019

@koute Do you have any other breaking changes you'd like to do before I do a release?

@koute
Copy link
Contributor Author

koute commented May 9, 2019

I don't have anything else planned at the moment, so you can go ahead with the release. (:

@sdroege
Copy link
Owner

sdroege commented May 9, 2019

Ok great, I'll probably get to that this weekend. Thanks again

@koute
Copy link
Contributor Author

koute commented May 9, 2019

Thanks!

@sdroege
Copy link
Owner

sdroege commented May 11, 2019

Published on crates.io now :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants