Add InputStream and OutputStream classes and default object

[mgeier]

This is an alternative to #39. It includes most of the changes discussed there.

The main difference is that there are not 3 kinds of arguments (e.g. device, input_device and output_device) anymore. Instead, there is only one kind of argument (e.g. device) which can optionally hold a pair of values (first for "input", second for "output").

The attributes of the default object can also optionally hold pairs of values, eg. pa.default.channels = 2, 1. The most common case, however, will be to assign single values which are used for both input and output. To get/set single values for only one of input/output, pa.default["input"].channels and pa.default["output"].channels can be used, respectively.

Also, as discussed in #39, the functions default_input_device() and default_output_device() are incorporated into pa.default.device.

[bastibe]

I like the Stream/InputStream/OutputStream hierarchy. This seems cleaner to me than the previous approach.

The Default-Value handling still seems convoluted though. I currently don't have time to work on that, but there's probably a more concise way of doing it.

[mgeier]

I also think the class hierarchy is better than in #39. Combining input_* and output_* and * to one single name also made things much simpler to use, in my opinion. I only had to add the helper functions _split() and _unique() to convert between single values and pairs.

I'm looking forward to your suggestions for making it more concise!

[mgeier]

I added a commit (81d0d98b8bf494963302e35f4afccc03a208b4e6) that removes the dubious descriptor class and replaces it by a "normal" class, which hopefully makes the code easier to understand.

The setters still accept single values as well as pairs of values, but the getters now always return pairs, even if the two values are equal. I think this is necessary to allow a sane implementation, but actually the behavior isn't that bad, either. It may be even superior to before.

[mgeier]

You're raising an interesting point here. I think prior to Python 2.6 it was clear that the EAPF style is more Pythonic and isinstance() should be avoided. Since the introduction of abstract base classes this has changed and now the documentation says:

"Note, however, that duck-typing can be complemented with abstract base classes."

Also, we have to use isinstance() anyway to handle strings separately. So yes, I think we should check for an ABC, but why would you still not support collections.Iterable? We would just have to add one call to tuple() to the implementation. And as a (probably good?) side effect, this would force a copy of the data and make the values immutable.

I think we can drop the indexing. Or does it make any (intended) difference?

And do you really want to allow length-1-sequences (and length-0-sequences) and ignore the rest if more than 2 items are given? Wouldn't it be more helpful and less error-prone to raise an error in those cases?

In other words:

if (isinstance(value, collections.Iterable) and
        not isinstance(value, str)):
    value = tuple(value)  # convert iterable to sequence
    if len(value) != 2:
        raise ValueError("Iterables must have exactly two items")
    return value
else:
    return value, value

[bastibe]

It makes no sense to support iterators-that-are-not-sequences, and then check for their length or convert them to tuple. That defeats the purpose of them not being sequences.

It's true though that we don't have to support sequences longer than 2:

if (isinstance(value, collections.Sequence) and 
    not isinstance(value, str) and
    len(value) == 2):
    return tuple(value)
else:
    return value, value

[mgeier]

Do you really not want to raise an error for len(value) != 2?

It makes no sense to support iterators-that-are-not-sequences, and then check for their length or convert them to tuple. That defeats the purpose of them not being sequences.

I don't get it. It makes perfect sense to me. For example, array.array() from the standard library accepts an iterable and immediately converts it to something that has a length.

Why would you want to force a user who happens to have an iterable with two elements to convert it to a sequence before being able to use it with PySoundCard?

I don't want to make PySoundCard deliberately less flexible without having a good reason. And up to now, I haven't seen a good reason.

I think it is a better design decision to allow iterables, but if you insist on not supporting them anyway, just say the word!

[bastibe]

Show me a usage example, then, that requires iterables-that-are-not-sequences. This code is specifically about making one-or-two things always-two things. We don't need to make this into a generalized multi-assignment facility.

[bastibe]

I'd like to split this discussion in half. Let's do the class structure refactoring first, and talk about the device unification separately.

This means that StreamBase needs to know two devices, both of which are simple dictionaries or classes. I realize that I am not comfortable with the other change yet.

[mgeier]

Show me a usage example, then, that requires iterables-that-are-not-sequences.

I don't have an example, that's the beauty of it!

I think when designing a library API, we shouldn't limit it to the use cases we can currently imagine.

In this context, I like the saying of Bjarne Stroustrup (he uses this regularly in his talks, but I couldn't find a better source right now):

"... that allows programmers to express ideas beyond what I can imagine ..."

He is of course talking about language design, but I think the same holds for API design.

Apart from that, I really like how Python embraces the concept of "iterable" within the core language. This is one of the design decisions that make Python code really beautiful. Iterables are very Pythonic. Therefore, I try to define APIs in terms of iterables rather than sequences wherever reasonable.

Of course, more genericity often has a cost (that's why YAGNI is a good principle in application code). In our case that's the cost:

value = tuple(value)  # convert iterable to sequence

One very simple line. That's not a lot. We now have to decide if it's worth adding this one line.

As I said, IMHO it's the better design choice to not unnecessarily restrict the API to sequences, but if you insist on this restriction (as a stylistic choice), just say it.

This is to a large part a matter of personal preference, and if we can't find an agreement, it's your job to decide.

Let's do the class structure refactoring first, and talk about the default object separately.

That's not so easy, the class implementation depends on the default values.

Scattering the default values all over the place is definitely a code smell, so they should probably be moved to a common location (probably at the module level). That's what I tried to do with the default object.

I'm open for suggestions how to improve the handling of default values, but I'm not willing to spend/waste time/energy/work in writing and testing an inferior implementation just to change it back to something reasonable right afterwards.

I realize that I'm not comfortable with the default object yet.

That's OK.

Any suggestions for improvement?

Any details that are especially uncomfortable?

[mgeier]

I have yet another suggestion:

def _split(value):
    """Split input/output value into two values."""
    if isinstance(value, str):
        # iterable, but not meant for splitting
        return value, value
    try:
        invalue, outvalue = value
    except TypeError:
        invalue = outvalue = value
    except ValueError:
        raise ValueError("Only single values and pairs are allowed")
    return invalue, outvalue

This is incidentally much closer to the original implementation before daa9d0ffa7848ee2604c2a77223f691962d8399f.

I think this is up to now the most readable (a.k.a. Pythonic) and overall "best" solution. It also doesn't impose any unnecessary restrictions on the user. And it always returns a tuple. Neither the type tuple nor Iterable/Sequence are explicitly mentioned, which I think is a good thing.

[bastibe]

This is a really nice solution for _split! Thank you for figuring this one out!

Regarding the default object:

• It is only read at Stream construction time, and later changes to the default object don't affect existing Streams. This is confusing, since it means that there are two separate places for configuring Streams: The default object, which affects Streams before a Stream is created, and the Stream object, after Stream creation.
• I don't like how most constructor arguments of the Stream classes can either be explicitly mentioned, or get taken from the default object. There should be one obvious way to do things. Either we use the default object exclusively or we use the Stream constructors. Maybe we can refactor this so that the user has to pass **defaults to the Stream constructor to get the desired result.

We should do another Skype call to figure this one out. Your current solution works well within the current framework. We should probably make a list of all the metadata required to open a Stream in portaudio, and try very hard to minimize the amount of user decisions.

[mgeier]

Most of the changes from this PR were done in #51. The default object was removed, but it may be re-introduced with #53.

I'm closing this PR, any further discussion should happen in #53 (or elsewhere).