-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Refactor usage of 'err()' with miniaudio results #3013
Refactor usage of 'err()' with miniaudio results #3013
Conversation
Maybe we can discuss what API changes you're thinking of first. On its own, I find this less readable, as I have to go and look up what printErr does exactly, plus it potentially removes some flexibility, in case you wanted to stream-out more information. This seems to outweigh the repeated |
First step would be to hide With that, an explicit I also want to explore a format-based API in the future for All of these changes would end up affecting every single usage of |
Technically, miniaudio returns |
Pull Request Test Coverage Report for Build 9103444480Details
💛 - Coveralls |
That would couple us even further to a stream-based API and would not help to make it easier to make changes to |
I don't think it is a strong argument that this change makes changing I would just bite the bullet, make the |
I only see that as an added benefit, in my opinion the PR has value regardless of that as a general DRY refactor. |
I'd rather we solidify our final error logging API before making changes like this. I summarized all my thoughts on the matter in this comment. |
Not worth arguing over this one. |
Refactors the commonly used
err() << "...message..." << ma_result_description(result) << std::endl;
pattern into
This is not only nice to avoid some repetition and gain conciseness, but I am planning to make future changes to the
err
API that, after this PR, would only need to be applied once for all miniaudio error reporting code.