SoundFile objects logically evaluate to False

[tgarc]

I noticed that SoundFile objects always evaluate to False. For example:

wf = sf.SoundFile('input.wav')
if wf:
    print("cool wav file man!")
    # never executed
else:
    print("no wav file...not cool.")

I don't know where this is coming from, I think by default classes inheriting from object will evaluate to True. Is this intended behavior? It's often convenient to rely on the object logically evaluating to True; for one thing builtin file objects evaluate to True so I would expect this behavior from SoundFile as well. Would you be alright with adding a bool method to return True?

[bastibe]

Interesting! On my computer, SoundFiles evaluate to true.

What version of SoundFile and Python are you running on which operating system?

[mgeier]

This seems to be because Python treats empty things as falseish.

@tgarc You should try it on a non-empty file, and you @bastibe on an empty one.

Is this intended behavior?

I guess up to your discovery it was accidental behavior.

builtin file objects evaluate to True

That's true, even for empty files. Because they don't support len() ...

Would you be alright with adding a __bool__ method to return True?

I don't think that's the right solution.

I think the problem is that we provide __len__() for something that's not an actual sequence.
We should remove __len__() and replace it by something like SoundFile.frames, analogous to the SoundFile.channels we already have.

Of course this would break backwards compatibility, so we should think about if it is worth it and at least provide a migration path.

[bastibe]

We should remove __len__() and replace it by something like SoundFile.frames, analogous to the SoundFile.channels we already have.

In theory, I agree, but backwards compatibility is a problem.

It might be more interesting to use __bool__ for whether the soundfile is still open or not.

[tgarc]

Ah, it's because of __len__. Totally overlooked that. That brings up another question: what does SoundFile.__len__ give for files that are not seekable? Does that return -1? 0? Or does it raise an error? I will have to come up with a test for that but any of those results would be unexpected.

[mgeier]

@bastibe

It might be more interesting to use __bool__ for whether the soundfile is still open or not.

This doesn't sound intuitive to me at all.
Is there a precedent for such a behavior?

We already have the property SoundFile.closed which is perfectly aligned with Python file objects, isn't it?

https://docs.python.org/3/library/io.html#io.IOBase.closed

@tgarc

It's often convenient to rely on the object logically evaluating to True.

This sounds kinda sensible, but do you also have a concrete use case in mind?

what does SoundFile.__len__ give for files that are not seekable?

Such files can still have a valid length.
The little helper function _update_len() should take care of non-seekable files that are written to.

But there might still be some edge cases where the reported length is garbage. I don't know.
If you find some, please speak up!

[tgarc]

This sounds kinda sensible, but do you also have a concrete use case in mind?

An example in my case is I want to check an input to a function - let's say it's called inpf - to see if a SoundFile was passed in or if inpf is just the default None value. It's an easy fix, I just have to check if inpf is None instead of if inpf. So I would consider this is a very minor issue, but I still think most users would see this as surprising behavior.

But there might still be some edge cases where the reported length is garbage.

I don't know about the write case, but in the read case I sometimes need to pipe input from different audio applications. (Think reading from an audio stream of unspecified origin). If I try opening STDIN as input to a SoundFile without a specified length (the format being RAW or AU) I get a garbage __len__ of 2**60 - 1 on my particular 64bit linux box. (Just FYI, to open stdin I pass in sys.stdin.fileno() or just the string '-').

I tend to agree that the "right" solution would be to drop __len__ altogether. But at this point it's a matter of 'is it worth changing it?'. On one side, SoundFiles more often than not have a sensible __len__ which has a consistent meaning (number of frames). On the other, an empty soundfile evaluates to False (odd), and in a small minority of cases __len__ returns a garbage value.

[mgeier]

@tgarc

in a small minority of cases __len__ returns a garbage value.

I think this is a separate issue and doesn't depend on whether we call it len(myfile) or myfile.frames.

Probably that's even a Libsndfile issue?
I think returning something like -1 for "unknown" would make more sense than returning a huge positive random number, wouldn't it?

For reference, this number was wrong before (but in a different context): libsndfile/libsndfile#77
We might be able to add a similar work-around here (see #68)?

Do you want to bring this up at https://github.com/erikd/libsndfile/issues?

[bastibe]

We should also consider that len is expected to be a positive integer. Anything else might be very confusing. If anything, we could think about nan (or indeed TypeError) in cases where len does not make sense. int32_max is not the worst solution, either. Negative lengths strike me as absurd, though--or is there a precendent?

[mgeier]

I would expect Python's len() function to only return non-negative integers.
If something doesn't have a length, I'd expect a TypeError: object of type 'SomeType' has no len().
I don't know of any Python object that only sometimes has a length.
This would speak for getting rid of __len__().

However, I don't think it's too absurd if the attribute myfile.frames is -1.
This would be consistent with the default value frames=-1 in sf.read(), sf.blocks(), sf.SoundFile.read() and sf.SoundFile.buffer_read().

I guess it's very common for C libraries to return -1 as error from functions that otherwise would return a length of some sort, but I don't know of any function in the Python standard library that returns -1 on error, I'd rather expect an exception to be thrown.

I'm not sure yet what makes most sense for the soundfile module, but above I was rather speaking about a potential Libsndfile issue. And there it would definitely make sense to use -1.
This would be consistent with sf_seek() returning -1 on error.
Since SF_INFO.frames is a signed integer this wouldn't be a problem.

---

In the case of the soundfile module, I think it would be strictly speaking most correct if we would provide neither __len__() nor .frames but rely exclusively on the seek() method.
But I guess this is a case of "practicality beats purity", right?

For reference, that's where we decided to go for __len__(): #69.

[tgarc]

Okay, so we're sticking with len; fair enough. Thanks guys.

[mgeier]

@tgarc I wouldn't close that yet ... I'm actually in favor of getting rid of __len__() and replacing it with something like .frames. Of course this would break existing code, so we would have to do a smooth transition over a long time, but I still think it's doable and in the long run it would lead to a slightly better library.

@bastibe What's your opinion?

Also, there is still the unrelated issue where a garbage value is returned. Do you want to create a libsndfile issue for that, @tgarc?

[bastibe]

Essentially, I think that this discussion has shown that the collection interface (len and indexing) is not a good match for soundfile, since not all soundfiles are seekable, and not all soundfiles have a definite length. Our inclusion of __len__ stems from an earlier time when we supported indexing on soundfile objects.

Instead, we can use the file-like object interface. At the moment, we implement almost the full interface of io.IOBase, except for fileno, isatty, readable, readline, readlines, writable, and writelines. We should be able to include at least readable and writable, and maybe fileno without much problems. To be more uniform, we would have to add a few errors when reading from a non-readable file or from a closed file etc.

In the io.IOBase interface, some methods return a value or throw an error if they are not applicable. If we implement .frames as a method, it would be normal behavior to throw an error if the file is not seekable.

Seeing that this would make the library more file-like and more consistent, I am in favor of removing __len__, and implementing some variant of .frames.