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

Simplify deriving from _StreamBase #65

Merged
merged 1 commit into from Jan 6, 2017
Merged

Simplify deriving from _StreamBase #65

merged 1 commit into from Jan 6, 2017

Conversation

mgeier
Copy link
Member

@mgeier mgeier commented Jan 4, 2017

People who want to create their own callback-based stream classes can directly inherit from _StreamBase, avoiding the unnecessary (and non-functional) read() and write() methods.

Also, if desired, they can quite easily implement a single stream class that (based on constructor arguments) supports only inputs, only outputs, or both inputs and outputs.

@mgeier mgeier merged commit 1aec2d6 into master Jan 6, 2017
@mgeier mgeier deleted the refactor-streambase branch January 6, 2017 19:23
@tgarc
Copy link
Contributor

tgarc commented Jan 11, 2017

cool, being able to easily subclass StreamBase will be a big help for me. It looks like if you set 'kind' to None that means full duplex correct?

Neat trick with the usage of _remove_self() BTW

@mgeier
Copy link
Member Author

mgeier commented Jan 11, 2017

Thanks!

And yes, kind=None is supposed to mean "duplex".
Before that PR, I was actually using the string 'duplex', but I had the feeling that this is inconsistent, because on some other occasions I was using the values ('input', 'output', None) for kind. In those cases None wouldn't mean "duplex" but "either of those two" or probably "both".

Would you think it's easier to understand if I use 'duplex' again?

Also, I don't know of the name kind even makes sense, since I'm not a native speaker.

If you have any other suggestions or comments, please tell me!

@tgarc
Copy link
Contributor

tgarc commented Jan 11, 2017

kind=None is a little confusing, 'duplex' makes more sense. I was thinking kind could be replaced with mode and input/output/duplex replaced with 'r'/'w'/'rw' but it's a minor detail. It's more important that it's consistent.

@mgeier
Copy link
Member Author

mgeier commented Jan 12, 2017

The problem with 'duplex' is that I'm using the kind argument also for query_devices(), where it doesn't really make sense.

I'm also using it for the internal functions _get_stream_parameters() (where None is not allowed) and _get_device_id(). But this is probably not very relevant ...

Do you think I should name those different occurrences of kind differently?

I don't really like the mode argument and the similarity to file open modes (although the name itself would be good).
Even if we would use the file metaphor, there is another problem: 'rw' doesn't exist in Python: https://docs.python.org/3/library/functions.html#open!

@tgarc
Copy link
Contributor

tgarc commented Jan 15, 2017

I wasn't trying to suggest that we treat the streams as file objects - they're not even file-like - which is why I didn't suggest a direct analog to the python file mode argument types but still, I can see how it could be confusing.

Perhaps something more explicit like stream_type would be clearer? That would make sense across the functions you mentioned. The fact that duplex doesn't make sense for query_devices() is kind of a corner case, you can just make a note in the docs that 'duplex' is not a valid argument for that function.

@mgeier
Copy link
Member Author

mgeier commented Jan 16, 2017

Thanks for your suggestions, I very much appreciate them.
But ... I don't like stream_type, because IMHO it does not make sense for query_devices()!
When querying devices there is no "stream". And even if I'm thinking about a "duplex" stream, I might still want to separately check for "input" and "output" devices. If anything, it would be a device_type, which sounds repetitive (query_devices(device_type=...)). It might as well be just type, but that's mirroring the built-in type() function.

Do you know any other alternatives for kind?

Regarding 'duplex', I think you convinced me that it makes more sense than None in the context of the _StreamBase constructor.
I'm re-introducing it in #66. There are already two different sets of possible values for kind (with and without None), I might as well add a third set (with 'duplex').

#66 would also be a great opportunity to change kind.

@mgeier
Copy link
Member Author

mgeier commented Jan 26, 2017

I've merged #66 just now. If you know a better name for kind, I'd still be interested ...

@tgarc
Copy link
Contributor

tgarc commented Jan 26, 2017

Can't think of anything. device_type is still probably better than kind but maybe not worth the commit. The only other thought I had was using mode but instead of r, w, or rw, use i, o or io. I think this doesn't make in the context of query_devices but there you can just use device_type, devtype, or something similar. maybe a bit redundant but it's still clearer.

so for example, for instantiating a stream class, instead of __init__(kind=None, ...) it would look like __init__(mode='io')

Personally I think it's cleaner and clearer but again, just my two cents :)

@mgeier
Copy link
Member Author

mgeier commented Jan 27, 2017

So am I understanding correctly that you are suggesting to use {'input', 'output', None} for query_devices() and {'i', 'o', 'io'} for _StreamBase?

@mgeier
Copy link
Member Author

mgeier commented Jan 27, 2017

Note that this is further used in _get_stream_parameters(), where it is used in the "default" mechanism, e.g. default.device[kind]. To change this, I'd have to change the class _InputOutputPair, too.

I'm also doing things like info['max_' + kind + '_channels'] and info['default_' + latency + '_' + kind + '_latency'], where kind must be exactly one of {'input', 'output'} for it to work.
With your suggested change I would either have to change the info keys to 'max_i_channels' (which would be backwards incompatible) or make the program logic more complicated.

I'm really open to (and thankful for) your suggestions and I don't want to seem dismissive, but the use of the strings 'input' and 'output' is really quite interwoven (if that's a word) in the whole implementation. Changes are of course possible, but it's much more than simply changing the set of possible values for an argument of a single undocumented function.

@mgeier
Copy link
Member Author

mgeier commented Feb 5, 2017

@tgarc Any more thoughts?

I would like to make a new release soon and if I change anything kind-related I would like to do it before that.

@tgarc
Copy link
Contributor

tgarc commented Feb 5, 2017

I suppose that I still think mode makes more sense than kind at least in the context of the stream constructors (while still using 'input'/'output'/'duplex'). At least that's a change that can be made while still keeping things mostly backwards compatible. The kind argument in query_devices seems kind of peripheral to me, I think we can do without changing the name there.

PS I'm all for a new release, I'd like to see this PR in particular be released for my own selfish needs ;)

@mgeier
Copy link
Member Author

mgeier commented Feb 6, 2017

Yes, if it's only changing a parameter name in a non-documented class (namely _StreamBase), it would be OK for me.

I'm still not 100% convinced, so I'm reluctant to implement it myself.
But if you make a PR and afterwards you are still convinced that mode is better than kind, I'll merge it.

After that and #73 is done, I'll make a new release.

@mgeier
Copy link
Member Author

mgeier commented Feb 14, 2017

@tgarc I've merged #73. Any news here?

@tgarc
Copy link
Contributor

tgarc commented Feb 14, 2017

I decided that I can't really justify making this change to the API. I'm going to leave it as is.

@mgeier
Copy link
Member Author

mgeier commented Feb 14, 2017

OK, that's fine by me.

Thanks anyway for the discussions, I found it very interesting.

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.

None yet

2 participants