API: Return a proper error type instead of `String`

[LunarLambda]

String does not implement std::error::Error, making interoperability with error-handling crates such as failure or anyhow much more difficult.

SDL2 already has an Error enum, with variant SdlError(String), we just need to use it.

Right now, to use anyhow's Context extension, for example, one needs to do the following:

use anyhow::{Context, Result}

fn main() -> Result<()> {
    let sdl = sdl2::init().map_err(sdl2::Error::SdlError).context("Failed to initialize SDL lbirary.")?;
}

unrelated, but why is render::SdlError a seperate newtype around a String, independent from the above mentioned Error enum (which has an un-newtyped variant of the same name)? It is only used by TargetRenderError.

[Cobrand]

I agree with what you are saying, but it's a breaking change so we have to change every single function affected in one go, and also explain in the readme how to fix the breaking changes exactly.

[LunarLambda]

I understand that this is a major breaking change, but seeing as we are pre-1.0 and this is an example of highly unidiomatic API design, and interacts poorly with other libraries, I think it is worth pursuing.

If SdlError was not exposed, the map_err call would look like this:

sdl2::init().map_err(|s| io::Error::new(io::ErrorKind::Other, s))

or, worse, if mis-using io::Error for this purpose was undesirable:

sdl2::init().map_err(<Box<dyn error::Error + Send + Sync> as From<String>>::from)

If most (or all) SDL functions only return errors in the form of strings, there should be reasonably small impact to people's actual code, as people would only have to update their return types, or, if they are already using crates such as anyhow, they can actually remove additional code like the above map_err calls now.

As for the concrete definition, I'd say something like this (based roughly off of io::Error) should suffice:

#[derive(Clone, Debug)]
pub struct SdlError(String);

// if construction from an arbitrary string would be desired (why?),
// we can add `pub fn new(string: String) -> Self { Self(string) }` to this 
impl SdlError {
    // N.B that this will return an empty string if no error has occured or `clear_error` was called.
    // perhaps this function could return `Option<Self>` instead,
    // and library code can `.unwrap` to assert that we only return `Err` when an error has actually occured.
    pub fn from_last_error() -> Self {
        Self(crate::get_error())
    }
}

use std::fmt;

impl fmt::Display for SdlError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "SDL Error: {}", self.0)
    }
}

// requires Debug (which we derive), and Display (impl above). The only other fn to implement would be `source`,
// but we don't have anything beyond what Sdl gaves us, so we can leave it at the default implementation.
impl std::error::Error for SdlError {}

I would be happy to work on implementing this change, although I would like to address the considerations raised in the comments above, and preferably receive a 'go-ahead' signal before starting.

[Cobrand]

I'm not saying this is an unnecessary change, I think it would be quite nice to have, just be extra careful with what gets broken and how to smooth the experience a maximum for the end users that get to experience this breaking change.

// if construction from an arbitrary string would be desired (why?),
// we can add pub fn new(string: String) -> Self { Self(string) } to this

Not sure if this is useful, so let's not add it by default. If we find a use-case where it's necessary, we can add it back then.

[Lindenk]

+1. Currently I'm just mapping all sdl Results to MyGameError::MiscError(String) because calling map_err on every single sdl call is insane, however this is less than ideal. Changing Result<_, String> to a dedicated error type wouldn't be much of a breaking change as the only change most people will need is impl From<SdlError> for TheirGameError {}

[TonalidadeHidrica]

I definitely agree that the returned should be some original concrete type, instead of being stringly typed.
If the compatibility is the issue, then how about adding a feature gate so that user can choose either approach?

[shish]

As an end-user, I would prefer "dealing with a breaking change and updating all my code to deal with good error handling" over "having bad error handling" (right now I'm just calling .unwrap() all over the place because it's too much of a pain to deal with Strings).

I too am happy to do the work (maybe even with a feature flag if we want compatibility) if there's a good chance of it getting accepted :)

[benbaarber]

Is this still alive? Great first impression of these bindings but the use of String as the error type might be a dealbreaker for me.