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

API: Proper error types instead of String #1424

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rniii
Copy link

@rniii rniii commented Jul 18, 2024

Work in progress. I am also taking time to unify all error types, since there were many SdlErrors before. I would like to discuss if compatibility is necessary, e.g. it could be a feature-flag that does type SdlError = String;

See also: #1053

Summary:

  • Removed OpenUrlError, PrefPathError, ShowMessageError, WindowBuildError and FontError in favor of Error::InvalidString
  • Removed common::IntegerOrSdlError in favor of Error::IntOverflow
  • Removed set_error_from_code (internal SDL api, shouldn't be here? could add back as SdlErrorCode)
  • Removed usage of NulError to Error::InvalidString?
  • Fixed pref_path checking for empty string instead of null

TODO:

  • Figure out TargetRenderError, TextureValueError, UpdateTextureError, UpdateTextureYUVError, AddMappingError
  • ttf::InitError could be removed, if same ref-counting as subsystems is implemented for it
  • ttf::FontError was removed, it might be good to review removing RendererableText, as a CString is always allocated anyway?
  • if compatibility is needed, ideally we would do String -> SdlError, however this is messy because RWOps should use Error, as it currently converts io::Error to String, and that should be avoided

@rniii
Copy link
Author

rniii commented Jul 19, 2024

I think I noted everything down, I'll leave actually changing all strings when I'm given the green light!

It is a breaking change, however migrating code should be pretty easy: Result<T, String>s should be changed to Result<T, sdl2::Error>, and custom error types (which from the thread people have been doing) should be updated too

Self::Sdl(msg) => write!(f, "SDL error: {msg}"),
Self::Io(err) => write!(f, "IO error: {err}"),
Self::IntOverflow(name, value) => write!(f, "Integer '{name}' overflows: {value}"),
Self::InvalidString(name, nul) => write!(f, "Invalid string '{name}': {nul}"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the definition the first field is the NulError, not the name.

To prevent these bugs (in this crate but also user code using this crate) I advise to use a struct like variant instead of a tuple like one. This also adds context to the context-less &'static str field. I suggest to change the definition to InvalidString { nul_err: NulError, var_name: &'static str } (or something like this) and roughly the same for IntOverflow.

impl error::Error for Error {
fn source(&self) -> Option<&(dyn error::Error + 'static)> {
match self {
Self::Sdl(..) | Self::IntOverflow(..) => None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source may also be returned for Self::Sdl

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.

2 participants