-
Notifications
You must be signed in to change notification settings - Fork 290
Rename sink and stream #824
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
Conversation
36e24b4 to
6823e06
Compare
|
No need for an in depth review, more requesting it to be sure you agree with the idea of the changes. The content of the PR is just a find replace. |
|
oh actually this needs a change-log entry :). I'll go ahead and add that. |
|
I like the change from However, I am less sure about the rename from
I suggest to rename to Instead of
|
Then those could be FileSink or NetworkSink.
I don't agree, the default right now is mixing. Yes that works for all but it also wastes cpu cycles for all. If you just need a queue you should not be forced to go through the mixer API and pay the runtime cost of mixing.
I'm okay with an OS -> Device rename.
Are you suggesting we have an "all encompassing" Sink struct? I would be against that. It seems useful to have a clear distinction in sinks. If users truly need to abstract over those we could offer them a trait (they may Box it themselves).
I have a little rodio experiment going on where I have: SinkBuilder::play(source: impl Source);
SinkBuilder::queue();
SinkBuilder::mixer();Mixer is just shorthand for: let (MixerSource, MixerHandle) = mixer::new();
builder.play(MixerSource);
return MixerHandle;How about we go with: |
I agree and have their own builders.
I agree that it's wasteful and shouldn't be necessary.
Yes I was also thinking a trait, so that a user can plug in a
Looks good. |
|
wonderful! I'll go and make the changes now :) |
As discussed in #815 (comment) this renames parts of rodio so they actually make sense.
This builds upon: #815, it should be merged first
PS: ignore branch name, I wanted to introduce a new source trait here but the rename takes priority.