Skip to content

Conversation

@mgeier
Copy link
Contributor

@mgeier mgeier commented Nov 25, 2014

No description provided.

@mgeier
Copy link
Contributor

mgeier commented Jun 15, 2014

This would be seek(0, SEEK_CUR), is it really worth making a separate method for that?

@bastibe
Copy link
Owner Author

bastibe commented Jun 16, 2014

I notice that I would use this function constantly during testing. Also, it is used four times internally.

@mgeier
Copy link
Contributor

mgeier commented Jun 18, 2014

It's not as easy as I thought because there is another option 'r'/'w' to manipulate the read/write position separately.

I notice that I would use this function constantly during testing.

This is a hint that we should add it, but would it be used as often in normal code?

It's really easy to create a convenience function within the tests (or in user code) for a specific set of fixed arguments.

I think "shortcuts" of this sort are really useful if they are targeted for use in an interactive session, but as I see it this is not the case here.

IMHO, this is part of the low-level functionality, therefore it doesn't have to be that easy.

Also, it is used four times internally.

I don't think this is a problem. In my opinion the seek() calls are easy enough to read.

I think we could add a tell() method, but this would increase the amount of documentation and simply the number of methods that are available, which may as well make it harder for users to find what they need instead of easier.

@bastibe
Copy link
Owner Author

bastibe commented Jun 18, 2014

There are two scenarios where tell would be useful:

  • in rw mode, where read and write require constant juggling of the respective read-pointers.
  • if you want to use a special data type, since that precludes the use of indexing.

Really, you need that functionality whenever I can't use indexing. I think that this is a common enough use case to warant its own method.

file.tell('w')

is just a lot nicer than

file.seek(0, sf.SEEK_CUR, 'w')

Frankly, I even think that

file.seek(file.tell()+5)

Is much easier to read than

file.seek(5, sf.SEEK_CUR)

even though it is somewhat less efficient.

@mgeier
Copy link
Contributor

mgeier commented Jun 18, 2014

I'm still not convinced.

  • in rw mode, where read and write require constant juggling of the respective read-pointers.

tell() wouldn't make this any simpler, it would just save a few keystrokes.

In my opinion, rw mode is one of the most advanced and at the same time least used features in PySoundFile.
So even if tell() would simplify the situation (which it doesn't), it would probably not be worth it anyway.

  • if you want to use a special data type, since that precludes the use of indexing.

Right after finishing the blocks() feature (#35), I will again try to convince you that indexing is a Bad Thing and we should abolish it.

We have a much superior way to read a single slice of data with the read() function and the frames/start/stop arguments, and soon we will have an also superior way to read slices successively with blocks().

And if you look closely at your argument, you'll see that it's really not so much for tell(), but against indexing!

But we don't have to discuss this topic now, I will bring it up later anyway.

Really, you need that functionality whenever I can't use indexing. I think that this is a common enough use case to warrant its own method.

read() and blocks() make indexing mostly obsolete.
In the very few cases where you want several possibly non-contiguous slices with possibly different block sizes I think it's OK if you have to do a few more keystrokes.

The differences in your example are mainly a matter of taste.
And if there are any differences beyond that, they are minuscule.

Having said all that, I want to stress that I don't have really strong arguments against tell(), I just think the arguments for it are also very weak.

Now that I think of it, I actually have some arguments against tell():

  • more work on code maintenance
  • more work on documentation
  • more work on tests
  • unexpected method for users familiar with the libsndfile API
  • unexpected arguments for users familiar with the OS function tell() (because of the separate read/write pointers)

In case of doubt, I would drop or at least postpone this issue.

@bastibe
Copy link
Owner Author

bastibe commented Jun 18, 2014

And if you look closely at your argument, you'll see that it's really not so much for tell(), but against indexing!

You misunderstand my argument completely. If I can index, I need neither seek nor tell. On the other hand, if I can't index, I do need seek and tell. I can express tell as seek, but that does not remove my need for it.

Regarding your last paragraph, please stop that crap. More work is is NOT an argument. We're doing this for free and for fun. If too much work ever entered that equation, we wouldn't do it. Seriously. It's insulting.

I am telling you that I see a need for tell. You don't. That's fine by me.

Arguments for tell:

  • It would make my life easier, based on my usage of PySoundFile.
  • It is somewhat expected, since it is a usual companion of seek, read, and write.

Arguments against tell:

  • It is not part of the libsndfile API. Though I don't see how that is relevant.
  • It does not work like the OS function. But neither does seek or read.

We can postpone this and see if I see more need for this outside of the tests.

@mgeier
Copy link
Contributor

mgeier commented Jun 19, 2014

And if you look closely at your argument, you'll see that it's really not so much for tell(), but against indexing!

You misunderstand my argument completely.

I don't think I'm misunderstanding it. I'm just viewing it from a different angle.

If I can index, I need neither seek nor tell.

I agree.

On the other hand, if I can't index, I do need seek and tell.

You do need seek(), tell() is optional.

I can express tell as seek, but that does not remove my need for it.

I'm not sure what you mean by that. Do you mean you need a method called tell() with no arguments or do you mean you need some method with some arguments that tell you the current read/write positions?

Regarding your last paragraph, please stop that crap. More work is is NOT an argument. We're doing this for free and for fun. If too much work ever entered that equation, we wouldn't do it. Seriously. It's insulting.

I don't think it's crap.
And I fail to see how that's insulting to whom, can you please elaborate?

We're indeed doing that for free and for fun, but it's work nonetheless.

And I like to spend work rather on something I believe is useful and improves the library, than spend work on something I don't believe in.
So for me, "more work" is definitely an argument in this case.

A related argument, which I failed to mention, is apart from "more work" it would also simply mean "more code".
I try, whenever I add code, to think if the additional code is really worth it.
In the current case, I'm still not convinced that it is.

Arguments for tell:

  • It would make my life easier, based on my usage of PySoundFile.

If that's the case, you shall get it!

As I wrote above: "I don't have really strong arguments against tell()."

I was just hoping to understand your motivation rather than taking your word for it.

I'm still not convinced by any of the arguments.

  • It is somewhat expected, since it is a usual companion of seek, read, and write.

OK, that's a valid argument.

Arguments against tell:

  • It is not part of the libsndfile API. Though I don't see how that is relevant.

Someone who knows the API wouldn't expect it.

  • It does not work like the OS function. But neither does seek or read.

As I said, a weak argument ...

We can postpone this and see if I see more need for this outside of the tests.

Yes, I think that would be the best course of action in the current case.

@mgeier
Copy link
Contributor

mgeier commented Aug 28, 2014

tell() should raise an error on non-seekable files, see https://github.com/bastibe/PySoundFile/pull/67#issuecomment-52505662.

@mgeier mgeier modified the milestone: 0.6.0 Oct 26, 2014
mgeier added a commit that referenced this pull request Nov 25, 2014
@mgeier
Copy link
Contributor

mgeier commented Nov 25, 2014

I added a commit implementing tell().
The error on non-seekable files is the one raised by seek(): " RuntimeError: Seek attempted on unseekable file type."

Is it OK? Can I merge?

@bastibe
Copy link
Owner Author

bastibe commented Nov 25, 2014

Merge away!

@mgeier mgeier merged commit 978db77 into master Nov 26, 2014
@mgeier mgeier deleted the issue-44 branch November 26, 2014 10:09
mgeier added a commit that referenced this pull request Dec 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants