A preview of the don't panic branch

[dagit]

Please Do NOT merge this yet!

I wanted to give you a heads up about some of the changes I want to make. So far I've been pretty aggressive about looking for calls to panic! and replacing them with bail! from the error_chain crate. At the moment that's just sugar for return Err(...). I have yet to start using any real features of the error_chain crate. I'm saving that for a later refactor.

After I get rid of the panics, I know I need to look for other forms of panic such as unwrap().

I'm interested in any feedback you have about these changes. Things you want me to change. Changes you don't want me to make, etc.

My reason for being pretty aggressive about getting rid of panic is that if the C code could return an error then it would be nice to get a chance to handle that on the Rust side even if it makes the API seem less elegant.

Thanks!

[Cobrand]

Looks great to me!

I would have preferred some of these changes to be in separate PRs though, for instance the changes to HatState. It's true that it's related, (and you probably caught it because of this PR) however this change is much more than avoiding panics, it's also changing the structure as a whole.

From this results in PRs where a big feature emerges, but some "by the way" breaking changes happen as well. It also becomes hard to write the changelog when changes like that are added sneakily in PRs.

Concerning this PR itself, I've only looked at it very quickly but it looks like you made most of the results Result<_, String>. I know that everything sdl2 returns is a string, however it we can sometimes avoid returning as String and returning an enum instead (for pattern matching), then that would be much better. If we need to have one error_chain! invocation per file to cover every case, then so be it (I'm not even sure that it possible but I hope it is). If I have to choose between something done quickly that can be improved, and something iterated over weeks where we will be happy with our first release, I'll choose the latter any day.

I don't plan to merge this into master just yet, if I will be merging this, that will be into another branch entirely, which will be dedicated to rust-sdl2 1.0.0-alpha. That means that this feature will probably not be out publicly until 1.0.0; when 1.0.0 we will ready and we will abandon support for the pre-1.0 releases, then I'll merge this into master.

Quick question, you added some unwraps yourself, if it's intentional maybe you could comment on why an Err there can never happen? If it's not or if you intended to look at it later on, just ignore waht I said.

[dagit]

I would have preferred some of these changes to be in separate PRs though, for instance the changes to HatState. It's true that it's related, (and you probably caught it because of this PR) however this change is much more than avoiding panics, it's also changing the structure as a whole.

Okay. It shouldn't be too hard to separate that out. Anything else you want me to pull out?

Concerning this PR itself, I've only looked at it very quickly but it looks like you made most of the results Result<_, String>. ...

Right. I tried to allude to that in my blurb. I 100% agree that String should be the exception and that enums should be the rule. I think it will be easier for me to make those changes incrementally though.

Quick question, you added some unwraps yourself, if it's intentional maybe you could comment on why an Err there can never happen? If it's not or if you intended to look at it later on, just ignore waht I said.

At the moment, since I don't properly use error_chains support for derived Into, using unwrap was the easiest thing that compiled and doesn't make the code more panicy (those cases would have already blown up). Most importantly though, I plan to make multiple passes and get rid of all the function that can panic such as unwap().

This was really meant as a preview so that we could get on the same page about the changes as a whole.

Thanks for your feedback!

[Cobrand]

Did you abandon this feature in the end?

[dagit]

No. I've just been keeping busy with other things. It sounded like these changes wouldn't be merged until a long ways off version so I haven't made it a priority lately.

…

On Tue, Oct 31, 2017 at 00:23 Cobrand ***@***.***> wrote: Did you abandon this feature in the end? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#701 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXefWQj-LVR61-oBWwL3fa2pvnsdqRJks5sxsrZgaJpZM4PMTZ5> .