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

Return Result from AudioQueue::queue instead of bool #1189

Merged
merged 1 commit into from
Dec 31, 2021

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Dec 29, 2021

Hi, thank you for developing the nice binding library. I enjoyed trying some SDL2 features with this library.

I noticed that AudioQueue::queue is not so idiomatic Rust API. It returns if it succeeded as bool. This PR replaces the return value with more idiomatic Result type. I believe Result is better because

  • Rust compiler does not complain about missing error handling if it returns bool. Actually audio-queue-squarewave example missed the error handling.
  • When an error occurs, user needs to get the error message via sdl::get_error. This is not idiomatic error handling and the error message may be overwritten by other errors.

I'm not sure that this change is appropriate because I'm new to both rust-sdl2 and SDL2. Please feel free to reject this PR if it's not appropriate.

@Cobrand
Copy link
Member

Cobrand commented Dec 29, 2021

Looks good, but please don't change the signature without prior warning.

Add the function with Result as "queue_audio" or something like that, and deprecate queue with a message on how to migrate from queue to queue_audio.

@rhysd
Copy link
Contributor Author

rhysd commented Dec 30, 2021

Sure. I'll update my patch.

@rhysd rhysd force-pushed the queue-return-result branch from 5d4a7c0 to 5d8ba4a Compare December 30, 2021 02:46
@@ -750,6 +750,10 @@ impl<'a, Channel: AudioFormatNum> AudioQueue<Channel> {

/// Adds data to the audio queue.
#[doc(alias = "SDL_QueueAudio")]
#[deprecated(
since = "0.36.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the next version is. If 0.35.2 is better, please let me know.

@Cobrand
Copy link
Member

Cobrand commented Dec 31, 2021

Thanks!

@Cobrand Cobrand merged commit 7011c6b into Rust-SDL2:master Dec 31, 2021
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