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

Avoid warnings under Python 3.10 #328

Closed
wants to merge 3 commits into from

Conversation

encukou
Copy link
Contributor

@encukou encukou commented Jan 13, 2021

This modernizes the code to avoid DeprecationWarning and ResourceWarning encountered in the test suite under Python 3.10a4:

  • load_module is deprecated
  • Some files weren't being closed

This changes the semantics of the compat.load_module function: on Python 3.5+, the module is no longer inserted in sys.modules. All non-test calls did del sys.modules[self.module_id] right after the call, anyway. On older Python, the module is inserted and then deleted.

(Some additional DeprecationWarning come from Setuptools: pypa/setuptools#2517)

@zzzeek
Copy link
Member

zzzeek commented Jan 13, 2021

this is great, let me try running this on CI which Im not even sure is working for Mako right now but will have to get this released.

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision c18d14d of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change c18d14d: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2470

@zzzeek
Copy link
Member

zzzeek commented Jan 13, 2021

you had a pep8 failure:


./mako/template.py:15:1: F401 'sys' imported but unused
import sys
^
ERROR: InvocationError for command /home/jenkins/workspace/pep8/sqlalchemy/mako/.tox/pep8/bin/flake8 ./mako/ ./test/ setup.py --exclude test/templates,test/foo (exited with code 1)
_________________________

can you force-push an adjustment ? thanks

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

Hi, this is sqla-tester and I see you've pinged me for review. However, user encukou is not authorized to initiate CI jobs. Please wait for a project member to do this!

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision c732cbf of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset c732cbf added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2470

spec = util.spec_from_file_location(module_id, path)
module = util.module_from_spec(spec)
sys.modules[module_id] = module
spec.loader.exec_module(module)
Copy link
Member

Choose a reason for hiding this comment

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

do we want to have:

        del sys.modules[module_id]

here? the idea is to not pollute sys.modules. seems to have all tests pass.

Copy link
Contributor Author

@encukou encukou Jan 14, 2021

Choose a reason for hiding this comment

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

Oh dear, I left sys.modules[module_id] = module in. I didn't mean to do that.
module_from_spec/exec_module doesn't put the module in sys.modules in the first place.

I removed the line and adjusted the test to check that sys.modules isn't polluted.

- In Python 3.5+, use spec_from_file_location(), module_from_spec()
  and exec_module() rather than the deprecated load_module().
  This avoids recording the imported module in sys.modules.
- Delete sys.modules[module_id] directly in load_module(), since
  that's what all call sites did.
- Adjust tests.
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 87c1d09 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

Patchset 87c1d09 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2470

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

Code-Review+2 Workflow+2

thanks so much for taking care of this!!

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2470

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/2470 has been merged. Congratulations! :)

@zzzeek
Copy link
Member

zzzeek commented Jan 14, 2021

mako 1.1.4 is released thanks very much !

@encukou
Copy link
Contributor Author

encukou commented Jan 14, 2021

Thank you for Mako! :)

@encukou encukou deleted the py310warnings branch January 14, 2021 19:46
@bourke bourke added this to Done in Mako prioritization Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants