sqlalchemy / mako

Mako Templates for Python
https://www.makotemplates.org
MIT License
353 stars 60 forks source link

Fix importing the test suite without lingua #357

Closed mgorny closed 2 years ago

mgorny commented 2 years ago

Defer the import of LinguaMakoExtractor in order to fix the following test error when lingua is not available:

ImportError while importing test module '/tmp/mako/test/ext/test_linguaplugin.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
test/ext/test_linguaplugin.py:5: in <module>
    from mako.ext.linguaplugin import LinguaMakoExtractor
mako/ext/linguaplugin.py:10: in <module>
    from lingua.extractors import Extractor
E   ModuleNotFoundError: No module named 'lingua'
jvanasco commented 2 years ago

This makes me ask a question to @zzzeek and @bourke -

lingua and babel are in the tox.ini for testing - https://github.com/sqlalchemy/mako/blob/main/tox.ini#L4-L14

however, they're not part of the setup.cfg for test -- https://github.com/sqlalchemy/mako/blob/main/setup.cfg#L44-L50

looking at how the babel plugin and test are structured, it is subject to the same error as this test.

IMHO, i think we could either (i) expand this "fix" to also cover babel (IIRC, dogpile uses the same technique with extension testing so it's not foreign to this project), (ii) expand the setup.cfg to install babel and lingua as part of the testing installation requirements, or (iii) both.

bourke commented 2 years ago

This makes me ask a question to @zzzeek and @bourke -

lingua and babel are in the tox.ini for testing - https://github.com/sqlalchemy/mako/blob/main/tox.ini#L4-L14

however, they're not part of the setup.cfg for test -- https://github.com/sqlalchemy/mako/blob/main/setup.cfg#L44-L50

looking at how the babel plugin and test are structured, it is subject to the same error as this test.

IMHO, i think we could either (i) expand this "fix" to also cover babel (IIRC, dogpile uses the same technique with extension testing so it's not foreign to this project), (ii) expand the setup.cfg to install babel and lingua as part of the testing installation requirements, or (iii) both.

When I refactored the test suite, I ran into the disconnect between tox.ini and setup.cfg. At the time I set it aside by telling myself the presence of tox.ini indicates that tests should run through tox. [It goes without saying here that we could do more to make policies around contribution and testing clearer, both in docs and here in the repo.] But I take your point about (iii) and agree it's the correct approach.

@mgorny, would you be willing to add support for the babel plugin to this PR? If you have other priorities I'll be happy to take it myself.

mgorny commented 2 years ago

I'm kinda busy right now, so I'd prefer if you took it. TIA!

bourke commented 2 years ago

I'm kinda busy right now, so I'd prefer if you took it. TIA!

No worries. Thanks for the PR!

zzzeek commented 2 years ago

gerrit is at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/3723

mgorny commented 2 years ago

Ping.

zzzeek commented 2 years ago

had to rebase will merge when the gerrit finishes tests

sqla-tester commented 2 years ago

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

zzzeek commented 2 years ago

ok we are good thanks

zzzeek commented 2 years ago

by which i mean, THANKS!!! :cake: :cake: :cake: