Rename seek() to seek_relative()? about python-soundfile HOT 15 CLOSED

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

from python-soundfile.

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.

from python-soundfile.

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

from python-soundfile.

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

from python-soundfile.

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.

from python-soundfile.

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.

from python-soundfile.

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?

from python-soundfile.

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

from python-soundfile.

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

from python-soundfile.

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'.

from python-soundfile.

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+'!

from python-soundfile.

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.

from python-soundfile.

I must say, all this seeking business is unexpectedly confusing.

Indeed!

I really don't like how seek now returns a tuple in "r+" mode.

But there are actually two different values to return ...

It seems that in libsndfile, if you do f.seek(n, sf.SEEK_CUR) on a rw file, the current position of the read pointer is ignored and both are set relative to the write pointer.
So in this case, the information about the read pointer is lost.

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.

I think you're right.
I implemented this in a new pull request #21, feel free to merge if you like it.

Then, we can still provide more fine-grained control with a seek_read and seek_write.

I don't like adding new method names if the behavior changes only slightly compared to an already existing method.
I would prefer to switch behavior with arguments in this case.

In #21, the read/write pointer can be set by OR'ing READ/WRITE into the mix, all this is just forwarded to libsndfile. The implementation of seek() is now just a one-liner, seek_absolute() is not needed anymore.

I also changed the mode handling: a3295f7. I used READ/WRITE/RDWR because that's closer to the actual constants in libsndfile. And I added the type _ModeType which causes the actual name of the constant to be displayed when returning it, e.g. from f.mode.
This is the same behavior as I've planned for the file formats and subtypes (see #18).

I abandoned the idea of using 'r+' instead of 'rw', for an explanation see a3295f7.

from python-soundfile.

I like it! The reasoning for 'rw' makes sense.

from python-soundfile.

Then merge away!

from python-soundfile.