Skip to content

Remove exclusive_creation argument from write() function#114

Merged
mgeier merged 1 commit intomasterfrom
issue-114
Mar 11, 2015
Merged

Remove exclusive_creation argument from write() function#114
mgeier merged 1 commit intomasterfrom
issue-114

Conversation

@mgeier
Copy link
Contributor

@mgeier mgeier commented Mar 10, 2015

This will revert the changes from #77.
Raising an error by default is useful in some cases, but it can also be harmful in other cases.

As far as I know, in most APIs the "write" function overwrites by default.
Also, e.g., the Unix commands cp and mv by default overwrite their target.

I think it's reasonable to assume that users expect overwriting behavior, therefore we should give it to them.

Once the default is changed to overwriting behavior, the argument exclusive_creation isn't that relevant anymore and it should be removed.

If users want to make sure that a file is not overwritten, they'll either have to check the existence first (e.g. with os.path.exists()) or they can use f = SoundFile(..., mode='x').

@mgeier mgeier self-assigned this Mar 5, 2015
@mgeier mgeier added this to the 0.7.0 milestone Mar 5, 2015
mgeier added a commit that referenced this pull request Mar 9, 2015
mgeier added a commit that referenced this pull request Mar 10, 2015
@mgeier
Copy link
Contributor Author

mgeier commented Mar 10, 2015

I added a commit that should implement the changes: 1ebcb89

@bastibe
Copy link
Owner

bastibe commented Mar 11, 2015

Shouldn't this change a test as well? If not, it should have ;-)

Anyway, looks good to me.

@mgeier
Copy link
Contributor Author

mgeier commented Mar 11, 2015

You're right, I forgot the tests, my bad!
There are actually 2 tests which fail now.

I'll fix the tests ...

@mgeier
Copy link
Contributor Author

mgeier commented Mar 11, 2015

I amended my commit, fixing one test and removing another (d39b332).

There is already a test case test_if_open_with_mode_w_truncates() which tests if 'w' and 'w+' truncate "normal" files, so I guess we don't have to make a separate test for sf.write(), right?

I hope now it's OK?

@bastibe
Copy link
Owner

bastibe commented Mar 11, 2015

Looks good to me. If you're happy with it, merge.

@mgeier mgeier merged commit d39b332 into master Mar 11, 2015
@mgeier mgeier deleted the issue-114 branch March 11, 2015 18:03
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.

2 participants