Skip to content

Conversation

@bastibe
Copy link
Owner

@bastibe bastibe commented Nov 28, 2014

  • Change order of format, subtype and endian function arguments in format_check, to be consistent with SoundFile.__init__ etc.
  • Change function argument order of blocks so that blocksize comes before the __init__ arguments. This would make the typical usage of this function easier.
  • Change function name of format_check to check_format.

@mgeier
Copy link
Contributor

mgeier commented Oct 14, 2014

  • Change order of format, subtype and endian function arguments in format_check, to be consistent with SoundFile.__init__ etc.

I don't think that's possible, because format is a required argument in format_check() (or rather check_format()).

OTOH, the admittedly strange order or arguments in the other functions has the reason that format is (I guess) in most cases specified by the file extension, allowing subtype to be specified as positional argument, e.g.

sf.write(myarray, 'myfile.wav', 44100, 'FLOAT')

I hope that's reason enough to have this strange order.

If not, we could change the order everywhere to format, subtype, endian (but I would vote against that).

  • Change function argument order of blocks so that blocksize comes before the __init__ arguments. This would make the typical usage of this function easier.

I'm not quite sure about this. I think I can live with either option.
I think it's nicely explicit to always type blocksize=, especially since there are 4 more numeric arguments to blocks() (not even counting samplerate and channels).
And it might be even interpreted as number of blocks, which isn't even available as an argument.
But sure it would save some typing in many cases.

Another argument comes up with 'RAW' files, but I guess that's uncommon enough to really matter:

for block in sf.blocks('myfile.raw', 48000, 44100, 2, 'PCM_16'):
    do_something(block)

vs.

for block in sf.blocks('myfile.raw', 44100, 2, 'PCM_16', blocksize=48000):
    do_something(block)

Now that I see it, this is really a weak point ...

  • Change function name of format_check to check_format.

I agree that sounds better.

@bastibe
Copy link
Owner Author

bastibe commented Oct 16, 2014

Regarding the second point, I was thinking that

for block in sf.blocks('myfile.wav', 1024):
    do_something(block)

is probably the most common use case.

For RAW files, I would probably either do

raw_format = {
    'samplerate': 48000,
    'channels': 2,
    'format': 'PCM_16'
}
for block in sf.blocks('myfile.raw', 1024, **raw_format):
    do_something(block)

or

for block in sf.blocks('myfile.raw', 1024, samplerate=48000, channels=2, format='PCM_16'):
    do_something(block)

But then, I have never actually used a RAW file anyway :-).

@bastibe
Copy link
Owner Author

bastibe commented Oct 16, 2014

If not, we could change the order everywhere to format, subtype, endian (but I would vote against that).

I agree. This makes no sense. RAW files require a subtype, but no format or endian. Thus, subtype should be first.

f = SoundFile('myfile.raw', 44100, 2, 'PCM_16')

I was thinking of unnamed things like streams and file descriptors, where subtype and endian can be deduced from the format, thus format should come first. However, these don't require a samplerate or channels, so format has to be given as keyword argument anyway:

f = SoundFile(stream, format='wav')

TL;DR: I agree on with you on the first point.

@mgeier mgeier modified the milestone: 0.6.0 Oct 26, 2014
@mgeier
Copy link
Contributor

mgeier commented Nov 16, 2014

We agreed that the first point should not be changed, the second point was fixed in 53aae5a (within #74), so the only missing point is to rename format_check to check_format.

@bastibe
Copy link
Owner Author

bastibe commented Nov 28, 2014

Does this look ok to you?

@mgeier
Copy link
Contributor

mgeier commented Nov 30, 2014

Yes, except that you seem to have forgotten the examples in the docstring ...

I forgot these two instances in the last commit
@mgeier
Copy link
Contributor

mgeier commented Dec 2, 2014

Great, let's merge!

bastibe added a commit that referenced this pull request Dec 2, 2014
Miscellaneous small changes
@bastibe bastibe merged commit 7b40774 into master Dec 2, 2014
mgeier added a commit that referenced this pull request Dec 17, 2014
The arguments which are forwarded to the SoundFile constructor, are
moved to the back (because they are very seldomly used).

See also #78.
@bastibe bastibe deleted the format_check branch April 25, 2016 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants