Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

FIX: deprecate qt4/5 rcparams #10351

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

Merged
merged 2 commits into from
Jan 31, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jan 30, 2018

PR Summary

#10282 deprecated rcparams backend.qt4 and backend.qt5 but left the defaults active so the deprecation warning came on, even if the user did not have these rcparams in the matplotlibrc. This PR makes the defaults None, and adds a check for None in the validator (that returns the old defaults). Hence the deprecation now only displays if these rcParams are still in the matplotlibrc.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak requested review from anntzer and efiring January 30, 2018 18:50
@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2018

Note that tutorials/introductory/usage.py is also now wrong.

# How do I select PyQt4 or PySide?

@jklymak jklymak added this to the v2.2 milestone Jan 30, 2018
@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 30, 2018
@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2018

Milestoning as release critical if #10282 is going to stay in...

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a much better way to do it indeed.

@efiring
Copy link
Member

efiring commented Jan 30, 2018

Failures:

/home/travis/build/matplotlib/matplotlib/lib/matplotlib/rcsetup.py:274:10: E711 comparison to None should be 'if cond is None:'
    if s == None:
         ^
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/rcsetup.py:287:10: E711 comparison to None should be 'if cond is None:'
    if s == None:
         ^

@jklymak jklymak force-pushed the fix-deprecate-qt45rcparams branch from 047aa18 to 17112e8 Compare January 30, 2018 21:10
@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2018

Sorry - shouldn't ask for reviews before I make sure the tests pass.

@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2018

@anntzer does this mean we can remove the somewhat complicated testing check? I left it in for now...

@anntzer
Copy link
Contributor

anntzer commented Jan 30, 2018

Probably the warnings suppression in matplotlib/__init__ too.

@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2018

I didn't see what needs changing in __init__.py...

@jklymak
Copy link
Member Author

jklymak commented Jan 30, 2018

I guess I wasn't saying we had to get rid of all the warning suppression. I don't quite understand what those two sections do. Feel free to push onto this PR, or open one and close thsi one.

@anntzer
Copy link
Contributor

anntzer commented Jan 30, 2018

Leaving them in is a bit redundant but not a critical problem I think.

@efiring efiring merged commit 5a58d8d into matplotlib:master Jan 31, 2018
@QuLogic
Copy link
Member

QuLogic commented Feb 4, 2018

Isn't this still happening on the Python 3.6 build? Sure is annoying when running pytest.

@jklymak
Copy link
Member Author

jklymak commented Feb 4, 2018

I’m a little confused about pytest. Does it look at the local matplotlibrc? If so, if you still have that rcParams in your matplotlibrc you’ll get the warnings.

@QuLogic
Copy link
Member

QuLogic commented Feb 4, 2018

Test setup calls rcdefaults, which re-applies all rcParams. I guess it takes the final result somehow instead of None.

@jklymak
Copy link
Member Author

jklymak commented Feb 4, 2018

Ok well we should put the test logic back then I guess. I do t quite get why this is happening though.

@anntzer
Copy link
Contributor

anntzer commented Feb 4, 2018

Won't have much time to take care of this (or other prs) in the coming days. Will get back later, but if this is blocking a release feel free to revert the original deprecation (sorry for the sloppy patch).

@jklymak jklymak deleted the fix-deprecate-qt45rcparams branch February 4, 2018 22:20
@jklymak
Copy link
Member Author

jklymak commented Feb 4, 2018

OK, backend.qt4 and backend.qt5 need to be added to _obsolete_set in __init__.py I think, though I'm a little confused about the differences between _obsolete_set and _deprecated_map.

@jklymak jklymak mentioned this pull request Feb 4, 2018
6 tasks
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.