New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `warnings.warn` everywhere and make mopidy "deprecation warnings safe" #1090

Merged
merged 25 commits into from Mar 31, 2015

Conversation

2 participants
@adamcik
Member

adamcik commented Mar 29, 2015

Fixes most of #1083. By deprecation warnings safe I mean that one should be able to run with -W error and expect things to work without crashing. Currently this is only true for the tests. Places we haven't switched to new APIs now have fat warnings in the form of catch_warnings contexts.

I've also switched to using the non-deprecated versions where practical of course. Through this I found that there are some cases where tracklist.add(tracks=...) still makes a lot more sense for now.

adamcik added some commits Mar 26, 2015

core: Mark library.lookup by uri deprecated
Updates core, mpd and tests to not use deprecated calls or safely catch them
when running with -W error.
tests: Cleanup mpd.protocol.current_playlist tests
- Split into smaller test cases more or less per command
- Created a BasePopulatedTracklistTestCase with a sensible setUp
- Modified test cases to work with the common tracklist state
- Replaced all calls to tracklist.add(tracks=...) with uris=...
- Test tracklist ordering in more compact way that also gives better error
  messages
tests: Ignore deprecated tracklist.add(tracks=...) in local tests
Note, this is mostly because these tests are just core tests in disguise and
need a lot more love than I can give them right now.

@jodal jodal added this to the v1.1 - Gapless playback milestone Mar 29, 2015

core: Add warning when doing library.search with a query.
Tests and code that rely on this are not yet "warnings safe".
@adamcik

This comment has been minimized.

Member

adamcik commented Mar 29, 2015

I've also added a warning for not using a query. But I kinda got sick of this cleanup, so I haven't yet cleaned those up. Also we have a separate bug for #1019 for this.

@@ -38,7 +38,8 @@ def config_override_type(value):
class _ParserError(Exception):
pass
def __init__(self, message):
self.message = message

This comment has been minimized.

@jodal

jodal Mar 29, 2015

Member

Another way of fixing this warning is to subclass mopidy.exceptions.MopidyException, which reimplements the message field that was deprecated in Python 2.6.

This comment has been minimized.

@adamcik

adamcik Mar 30, 2015

Member

I know, but since this is an "internal" error I decided against it.

This comment has been minimized.

@jodal

jodal Mar 30, 2015

Member

Fair enough.

def setUp(self): # noqa: N802
super(DeprecatedFindExactCoreLibraryTest, self).setUp()
self._warnings_filters = warnings.filters
warnings.filters = warnings.filters[:]

This comment has been minimized.

@jodal

jodal Mar 29, 2015

Member

Doesn't it read better when you make a copy to self._warnings_filters instead of assigning a copy to the same place, which looked at in isolation looks like a noop?

This comment has been minimized.

@adamcik

adamcik Mar 30, 2015

Member

This was simply copying how warnings.catch_warnings is done internally. Other option that we could consider is:

        self._catch_warnings_cm = warnings.catch_warnings()
        self._catch_warnings_cm.__enter__()

This comment has been minimized.

@adamcik

This comment has been minimized.

@jodal

jodal Mar 30, 2015

Member

My suggestion was simply to change from:

self._warnings_filters = warnings.filters
warnings.filters = warnings.filters[:]
warnings.filterwarnings('ignore', '.*library.find_exact.*')

To:

self._warnings_filters = warnings.filters[:]
warnings.filterwarnings('ignore', '.*library.find_exact.*')

This is both shorter, and to me it is also more obvious what is done.

This comment has been minimized.

@jodal

jodal Mar 30, 2015

Member

Going the pytest path, the way to do this is probably with a yield_fixture: http://pytest.org/latest/yieldfixture.html

This comment has been minimized.

@adamcik

adamcik Mar 30, 2015

Member

Mhm, I understood you suggestion. It just got me thinking if there was a more sane way to get all the tests to be run inside a context manager.

@@ -331,8 +338,9 @@ def listallinfo(context, uri=None):
if not lookup_future:
result.append(('directory', path))
else:
for track in lookup_future.get():
result.extend(translator.track_to_mpd_format(track))
for uri, tracks in lookup_future.get().items():

This comment has been minimized.

@jodal

jodal Mar 29, 2015

Member

uri is not used. Use .values() instead of .items()?

if not query:
warnings.warn(
'library.search() with an empty "query" argument deprecated',

This comment has been minimized.

@jodal

jodal Mar 29, 2015

Member

Missing "is" in the warning.

This comment has been minimized.

@jodal

jodal Mar 29, 2015

Member

This deprecation must be added to the docstring and 1.1 changelog.

This comment has been minimized.

@adamcik

adamcik Mar 30, 2015

Member

The empty query case we already added for 1.0, I presume you are talking about kwargs based queries?

This comment has been minimized.

@jodal

jodal Mar 30, 2015

Member

Both. The empty query being deprecated is not noted in the docstring either.

This comment has been minimized.

@adamcik

This comment has been minimized.

@jodal

jodal Mar 30, 2015

Member

OK, I must have missed it because I expected it to be together with the .. versionadded:: section or something.

if kwargs:
warnings.warn(
'library.search() with keyword argument query is deprecated',
DeprecationWarning)

This comment has been minimized.

@jodal

jodal Mar 29, 2015

Member

This deprecation must be added to the docstring and 1.1 changelog.

@jodal

This comment has been minimized.

Member

jodal commented Mar 30, 2015

Looks good.

What's the long term plan with all the ignored warnings? Make tracklist.add() take refs and use the refs returned from the future version of library.search()? In other words, not something we'll fix until 2.0?

@jodal jodal self-assigned this Mar 30, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 30, 2015

As for the longer term plans, it's basically try and move to refs and see where the API goes and continue reducing places we need to catch warnings.

utils: Create warn and ignore deprecation warning helpers
This moves all the deprecation warnings messages to a central place so that it
is easy to match against them without having to redefine the same regex all
over the place.

Each message has been given a message id which is more or less
module.func:extra-info. This is not intended to be parsed, just used in tests
when using the ignore helper.
@adamcik

This comment has been minimized.

Member

adamcik commented Mar 30, 2015

There, think that should cover all of it.

'core.library.search:kwargs_query':
'library.search() with "kwargs" as query is deprecated',
'core.library.search:empty_query':
'library.search() with empty "query" is argument deprecated',

This comment has been minimized.

@jodal

jodal Mar 30, 2015

Member

Sentence doesn't make sense

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 31, 2015

Fixed.

jodal added a commit that referenced this pull request Mar 31, 2015

Merge pull request #1090 from adamcik/fix/deprecation-warnings
Add `warnings.warn` everywhere and make mopidy "deprecation warnings safe"

@jodal jodal merged commit 9b03eee into mopidy:develop Mar 31, 2015

3 checks passed

Scrutinizer 131 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.21%) to 76.68%
Details

@adamcik adamcik deleted the adamcik:fix/deprecation-warnings branch Apr 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment