8000 Ensure the gc module is available during interpreter exit by embray · Pull Request #4166 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@embray
Copy link
Contributor
@embray embray commented Feb 25, 2015

This is directly related to #4165. Another side effect of run_setup is that any modules that are imported during the run_setup call are later removed from sys.modules. If there are no other references to that module they will be garbage collected and any previous references to globals in that module will be replaced with None.

Because destroy_all is registered as an atexit function it will run even after all the module itself has been garbage collected (there are other contexts where this might happen as well). This seems to otherwise work since it was changed to a classmethod, so now the Gcf class itself is still hanging around. But it may be necessary to re-import the gc module if it's to be made use of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, this is only in this method since it also is registered as an atexit function. Otherwise it wouldn't be necessary here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it would also be worth adding a comment that this is here deliberately (since gc is already imported at the module level any delinter is going to complain about this...)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good point.

Copy link
Member

Choose a reason for hiding this comment

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

the # noqa flag works on most linters?

@mdboom
Copy link
Member
mdboom commented Feb 25, 2015

Seems fine -- and pretty harmless in the 99.9% of non-corner cases where this is a non-issue.

@tacaswell tacaswell added this to the next point release milestone Feb 25, 2015
@tacaswell
Copy link
Member

👍 from me modulo a comment / linter protection flag.

@tacaswell tacaswell merged commit ea367e2 into matplotlib:master Feb 27, 2015
@tacaswell
Copy link
Member

Merged locally to add comment + linter flage

@embray embray deleted the patch-2 branch February 27, 2015 16:30
@embray
Copy link
Contributor Author
embray commented Feb 27, 2015

@tacaswell Where did you merge locally? GitHub just shows that you merged my PR as-is.

@embray
Copy link
Contributor Author
embray commented Feb 27, 2015

Or to be more specific, I'm wondering how you got GitHub to recognize this as merged, while also including your own changes. It would be lovely if I knew how to do that.

@tacaswell
Copy link
Member

Merged as 13a8f31

GH keeps track of 'mergestate' based on if the commits on the merge-branch are accessible from the target branch so all you have to do is merge locally and push to the target branch and GH is happy.

@stonebig stonebig mentioned this pull request Sep 2, 2015
17 tasks
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.

3 participants

0