Skip to content

deprecate indexing of SoundFile#75

Merged
bastibe merged 2 commits intomasterfrom
indexing_deprecation
Dec 2, 2014
Merged

deprecate indexing of SoundFile#75
bastibe merged 2 commits intomasterfrom
indexing_deprecation

Conversation

@bastibe
Copy link
Owner

@bastibe bastibe commented Nov 28, 2014

Using __getitem__ or __setitem__ should raise a warning, and the deprecation should be marked in the docstrings of SoundFile, pysoundfile, and the README.

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

mgeier commented Nov 16, 2014

OK, I guess the warning should work like this:

from warnings import warn
warn("blah", DeprecationWarning)

See https://docs.python.org/3.4/library/warnings.html#warnings.warn.

I think this is one of the few cases where the import should happen directly before using the function.

@bastibe bastibe force-pushed the indexing_deprecation branch from 26e6d91 to 501ccab Compare November 28, 2014 13:51
@bastibe
Copy link
Owner Author

bastibe commented Nov 28, 2014

This warning only shows up if you explicitly ask for it. Normal users will never see this. Is this really what we wanted?

@mgeier
Copy link
Contributor

mgeier commented Nov 30, 2014

Hmmm ... I wasn't aware of that. Apparently, since Python 2.7, DeprecationWarning is silenced by default.

I don't think this is what we wanted.

We could just get a bit more aggressive by removing the implementation and just raising a NotImplementedError.
And for the release after the next one we can completely remove the methods.

Alternatively, we could change the warning to a plain Warning, then it should be shown by default.

@bastibe
Copy link
Owner Author

bastibe commented Dec 1, 2014

They make a point in the documentation, that the way we are doing it right now is the preferred way of doing it, since users don't typically need to be aware of deprecations, and developers typically should be running programs with all warnings enabled anyway.

I agree that we should elevate this to a plain Warning.

since DeprecationWarnings are by default not visible to users, and this
is a user-visible feature.
@mgeier
Copy link
Contributor

mgeier commented Dec 2, 2014

Now the warning shows up, let's merge!

bastibe added a commit that referenced this pull request Dec 2, 2014
@bastibe bastibe merged commit 8d8c7b8 into master Dec 2, 2014
@bastibe bastibe deleted the indexing_deprecation 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.

2 participants