Change open mode and seek() behaviour

[mgeier]

See #10

[bastibe]

This is awesome! I think we finally arrived at a very good solution for this! Thank you so much for you continued work on these projects!

If I'm not missing anything, you don't seem to define READ, WRITE, or RDWR anywhere in the pull request?

[mgeier]

They're defined in the dictionary _open_modes in line 121.
They're added to the module namespace with _add_constants_to_module_namespace(_open_modes, _ModeType) in line 222.

I'm planning to use the same method for format/subtype/endian as sketched in #18.

The reason why I'm not assigning them directly in the module namespace is to be able to use the dict _open_modes in _ModeType.__repr__(). This way, known modes are displayed with their name, unknown ones just as ints. For the modes, however, there shouldn't be the possibility to use unknown numbers (except if there are bugs), for the formats this might happen if a user specifies a numeric format which is not known to PySoundFile (I don't want to prohibit this).
If the modes are used in Python code that expects integers, the constants still work.

Another advantage of the type classes is that they help checking for the wrong argument order.
If the second argument to __init__() isn't a _ModeType (or one of the allowed strings), a ValueError is raised.
For format/subtype/endian I'm planning to go the other way: if any of the (potentially) numeric arguments file, sample_rate or channels are given a format type by mistake, a TypeError is raised.
This way you cannot forget the channel:

f = sf.open('myfile.wav', 'w', 44100, sf.PCM_24)

Without the check, this would create a 3-channel file with subtype sf.PCM_16.

The only drawback I see to this solution (except it being von hinten durch die Brust ins Auge), is that pylint doesn't get it and says: E0602: Undefined variable "RDWR".

I'm glad you like the PR!

[bastibe]

Oh, I get it. Great!

I'm not sure I like that _add_constants_to_module_namespace solution too much. But I guess the function name is obvious enough. Still, it kind of violates the pythonic it's always clear where stuff comes from rule.

[mgeier]

Thanks for merging!

I didn't know the rule, I think it's a good one.
However, I had to break it to achieve these 2 goals (I'm using the file formats as example):

• constants should be directly in the module namespace, not hidden in some dictionary (to be able to do sf.WAVEX instead of sf.snd_types['WAVEX'])
• when returned in an interactive session, the values should be shown as symbolic names, not as numbers (e.g. f.format should return WAVEX instead of 1245184)

If you know a better way to achieve these, please tell me!

[bastibe]

You could just write

READ = _ModeType(0x10)
WRITE = _ModeType(0x20)
RDWR = _ModeType(0x30)

Or you could define the variables that way to begin with, like

READ = ModeType(0x10, 'READ')

and not have an _open_modes dictionary at all.

[mgeier]

The first example wouldn't work because _open_modes is actually accessed in _ModeType.__repr__().

Your second example looks promising, I think it could achieve both goals mentioned above.
However, I'm not sure if it would be a clearer or better implementation.
It would be more verbose because both READ and 'READ' have to be specified.
Also, each instance would have to hold the whole string in addition to the int value.

I guess we could live with this (although I think it's not nicer than the current solution), but the really bad thing would be that the class couldn't be used anymore within the code! No object of the type could be constructed from an integer (without specifying the string as well).
Admittedly, this is only used once for each of FormatType, SubtypeType and EndianType in #18 (in the respective properties around line 436), but I think it is quite useful there.

[bastibe]

You could define a type like

class NamedInteger(int):
    def __new__(cls, value, name):
        inst = super(NamedInteger, cls).__new__(cls, value)
        inst.name = name
        return inst
    def __repr__(self):
        return self.name

then, you could replace all the numeric constants with something like

NamedInteger(0x20, 'WRITE')

I haven't thought this through yet. But something like this might actually be a better solution than all those dicts.

[mgeier]

I finally came up with a solution based on your suggestions and I implemented it in 1a0c137.

For the open mode I'm using a classmethod to set up the id/string mapping.

For the formats I'm searching in the whole module namespace via globals(). This is less efficient than before, but I guess in this situation speed doesn't really matter.

I'm not sure, however, if this is really better than before ...

[bastibe]

I really think that having those constants explicitly defined in the module scope makes it much more readable. Having less "magic" is a good thing. You seem to be using autocompletion a lot. I usually read the code instead--not seeing those constants would confuse me a lot.

[mgeier]

I agree that it is better if we can easily see where the constants are defined.

And I'm using auto-completion only since recently, when I started working with IPython.
And I like it a lot!