Skip to content
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

Import Events and Matplotlib Interactive Fixes #2225

Merged
merged 4 commits into from
Feb 21, 2017
Merged

Import Events and Matplotlib Interactive Fixes #2225

merged 4 commits into from
Feb 21, 2017

Conversation

scopatz
Copy link
Member

@scopatz scopatz commented Feb 19, 2017

This PR adds six new events for hooking into Python's import mechanism. Now you can register a handler for before/after a module is search for, created, or exec'd. There are probably some pretty crazy abuses of this.

The initial application of this is to help fix some of the interactivity issues with matplotlib. This should close out #1043. Some of the problems mentioned in that issue seemed to related to mpl v1.x and have since been fixed in mpl v2.x. This PR tries to address the remaining problems.

@codecov-io
Copy link

codecov-io commented Feb 19, 2017

Codecov Report

Merging #2225 into master will decrease coverage by -0.13%.
The diff coverage is 51.47%.

@@            Coverage Diff             @@
##           master    #2225      +/-   ##
==========================================
- Coverage   29.37%   29.25%   -0.13%     
==========================================
  Files          95       95              
  Lines       15688    15709      +21     
  Branches     2926     2927       +1     
==========================================
- Hits         4608     4595      -13     
- Misses      10562    10594      +32     
- Partials      518      520       +2
Impacted Files Coverage Δ
xonsh/pytest_plugin.py 0% <ø> (ø)
xonsh/main.py 53.07% <100%> (ø)
xonsh/imphooks.py 40.76% <51.56%> (+7.91%)
xonsh/ansi_colors.py 0% <ø> (-18.31%)
xonsh/proc.py 53.73% <ø> (-0.15%)
xonsh/tools.py 27.53% <ø> (-0.05%)
xonsh/readline_shell.py 0% <ø> (ø)
xonsh/tracer.py 0% <ø> (ø)
xonsh/winutils.py 0% <ø> (ø)
xonsh/ptk/shell.py 0% <ø> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27a3775...395aa1e. Read the comment docs.

@AstraLuma
Copy link
Member

This seems absurdly dangerous. Are there other ways to accomplish this without new events? Or with events that are not so broad in scope and power?

@scopatz
Copy link
Member Author

scopatz commented Feb 20, 2017

I don't know that this is any more or less dangerous than what Python's import system already provides. Anyone could write an import finder and stick it at the start of the sys.meta_path that would totally disrupt the import mechanism, if they wanted. All this really provides is a translation layer between the two powerful systems. It doesn't stop bad behaviour but it does make good behaviour easier.

@melund
Copy link
Member

melund commented Feb 20, 2017

@scopatz : I can't get this working:

image

It just shows a blank figure. If I then run plt.show() the figure renders but then the prompt it blocked. Closing the figure returns the prompt. But if I press ctrl-c instead I get the following error.

snail@sea ~\Documents\xonsh ie
$ plt.show() (CTRL-C)
QObject::killTimers: timers cannot be stopped from another thread
PS C:\Users\mel\Documents\xonsh>

@scopatz
Copy link
Member Author

scopatz commented Feb 21, 2017

Thanks for trying it out @melund. I think this is almost certainly a matplotlib & windows issue. I know that they have had issues like this open in the past (matplotlib/matplotlib#3052). Perhaps it is best to report this there?

This PR does fix some of the issues I was seeing on Linux, though.

Having dived into the matplotlib code to write this PR, I am not convinced that their interactive mode is really well thought out. They special case a lot of stuff to work for just IPython. It would be much better to have a real API to work with.

@melund
Copy link
Member

melund commented Feb 21, 2017

Then I am OK with merging.

@melund melund merged commit 8acaa3f into master Feb 21, 2017
@scopatz
Copy link
Member Author

scopatz commented Feb 21, 2017

Thanks @melund! I do think this is worth reporting over at matplotlib. They seem to be trying to move towards something better. Bringing it up again couldn't hurt.

@scopatz scopatz deleted the ie branch February 21, 2017 04:20
@melund
Copy link
Member

melund commented Feb 21, 2017

I agree. Also maybe a new issue for xonsh so we don't forget. I don't have time to do it right now thus.

@scopatz
Copy link
Member Author

scopatz commented Feb 21, 2017

Sure, but let's close out the old bug and label the new one as a feature.

@melund
Copy link
Member

melund commented Feb 21, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants