Rename seek() to seek_relative()?

[mgeier]

This would seem more symmetrical, as there is also seek_absolute().

Using seek() without being explicit may have lead to bugs like these two:

https://github.com/bastibe/PySoundFile/blob/cd9fb6a287731288b43153cf243245943e88b54b/pysoundfile.py#L445

https://github.com/bastibe/PySoundFile/blob/cd9fb6a287731288b43153cf243245943e88b54b/pysoundfile.py#L471

In both cases there should be seek_absolute() instead of seek(), which is correctly used at the third place:

https://github.com/bastibe/PySoundFile/blob/cd9fb6a287731288b43153cf243245943e88b54b/pysoundfile.py#L423

BTW, this 3-fold repetition calls for refactoring, probably using a Context Manager ...?

[bastibe]

Very good catch! I'll hopefully have time to work on this this weekend.

[mgeier]

When you fix #11 and merge #12 there will be only one of the three instances left, therefore a Context Manager won't be necessary.

[bastibe]

The broken seeks should be fixed in 7a27d57 . Or did I miss something?

[mgeier]

Thanks. I tried it and it works perfectly as far as I can tell!

[mgeier]

I'd like to re-open this issue.

The bug which I described was fixed, but this was just a side note. The original issue was about renaming seek().

I strongly dislike the current situation (having seek() and seek_absolute()), because it is unnecessarily asymmetric and it is not how it works with normal files in Python. The default for the seek() method in Python is to seek from the beginning!

I see three possible ways to fix this:

• remove seek_absolute() and use seek() with the whence option. To be consistent with Python, whence=0 should be the default (os.SEEK_SET). This seems not Pythonic to me, but ironically this is used by the normal Python file objects.
• remove seek_absolute() and use seek() with another option, e.g. f.seek(123, absolute=False). The default should also in this case be absolute=True.
• provide seek_absolute() and seek_relative(), avoiding any doubts about default settings.

Obviously, I'd prefer the third option, but any of the three is better than the current situation.

[mgeier]

If the file is opened with SFM_RDWR, it get's even more interesting ...

The read and write pointer can be positioned separately by OR-ing the standard whence values with either SFM_READ or SFM_WRITE.

http://www.mega-nerd.com/libsndfile/api.html#seek

I think PySoundFile should also support this, but I don't know (yet) how this could be done in a Pythonic way.

[bastibe]

I like the whence idea! The more pythonic the better!

Regarding SFM_RDWR, this is pretty finicky anyway and does not work reliably with every sound file configuration. How about providing an additional keyword argument to seek that says seek(256, whence=128, which='rw'), which defaults to 'r' for read-only, 'w' for write-only and 'rw' for read-write SoundFiles?

[mgeier]

I'm sorry, but I don't get what whence=128 is supposed to mean in your example. Can you please explain?

The which argument would mean that both pointers cannot be set independently in one call.
What about providing the named arguments read and write?

seek_absolute(read=42)
seek_absolute(write=23)
seek_absolute(7)  # both read and write are set to 7
seek_absolute(42, 23)  # read and write are set individually

[bastibe]

Oh my, you are completely right. I misunderstood the meaning of whence. The example should have been seek(256, whence=os.SEEK_CUR, which='rw').

We might be overcomplicating things a bit. I think this feature is pretty obscure and probably of very little importance to most people.

That said, I don't like separate read and write arguments too much, since they would make the function signature hard do understand. The meaning of read and write in seek(read[, write, whence]) is not obvious if you expect seek(offset[, whence]). Also, it is confusing that read has a different meaning depending on whether write is assigned or not. If anything, I would go for seek(offset[, whence, read_offset, write_offset]), where *_offset would overwrite whence, if present.

How about using a tuple?

seek(7)        # both read and write
seek((7,))     # both read and write
seek((7,None)) # read only
seek((None,7)) # write only
seek((7,8))    # read and write separately

[mgeier]

I updated #18 (esp. commit 5a4d76e) before reading your response ...

I agree that the feature is a little obscure, but I nevertheless would like to have it in PySoundFile.
Especially the whence option is confusing, that's why removed it (as you did in your examples).

I also agree that messing around with the arguments in seek() will confuse users.
That's why I renamed seek() to seek_relative() ... incidentally that was my main point with this issue.
Now it's officially a new function and doesn't have to have any similarities to the arguments of seek().

In my proposal, the function signature is "officially" seek_relative(both) and seek_relative(read, write). Same for seek_absolute().
In reality, it's a little more convoluted, it's seek_relative(both, write, read), but both and read can never be specified at the same time. The first positional argument is actually shared between both and read. This is not perfect, but I think the function calls nevertheless look quite reasonable.
Ironically, without having read your response, both can also be a 2-tuple. This is mainly because the return value is also a tuple (but only if mode=='r+').

I agree that separate read and write arguments are a little ugly, but I think having to specify None explicitly is worse (and less explicit).

Here your examples again, augmented with a few more, according to my proposal. The comments specify if something is supposed to work or not.

seek_relative(7)        # both read and write if mode=='r+', read only if mode=='r'
seek_relative((7,))     # doesn't work
seek_relative((7,None)) # read only
seek_relative(r=7)      # read only
seek_relative((None,7)) # write only
seek_relative(w=7)      # write only
seek_relative((7, 8))   # read and write separately
seek_relative(7, 8)     # read and write separately
seek_relative(r=7, w=8) # read and write separately

Note that the return value is a tuple if mode=='r+' and a single int if mode=='r'

Note also that seeking is not at all possible if mode=='w'.

[mgeier]

And of course there is a bug! I fixed it in b00d052.

This idiom works always, regardless if curr is a tuple or an int:

curr = self.seek_relative(0)
# do something (including seeking)
self.seek_absolute(curr)

However, if something else has to be done with curr, special care has to be taken, e.g.:

curr = self.seek_relative(r=0)
# now curr is an int!

or

r, w = self.seek_relative(0)
# this only works if mode == 'r+'!

[bastibe]

I must say, all this seeking business is unexpectedly confusing. I really don't like how seek now returns a tuple in "r+" mode.

Honestly, I would prefer a pythonic seek with whence that does the standard Python thing to both the read pointer and the write pointer. I think for most use cases, this is the function we need and sticking to the Python default will avoid confusion. Then, we can still provide more fine-grained control with a seek_read and seek_write.