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

use standatd contextlib instead outdates contexter #421

Closed
wants to merge 1 commit into from
Closed

use standatd contextlib instead outdates contexter #421

wants to merge 1 commit into from

Conversation

kloczek
Copy link

@kloczek kloczek commented May 2, 2021

contexter module is not maintained since 2017.

Tested using pytest

$ /usr/bin/python3 -Bm pytest -ra tests/test_sessions.py
===================== test session starts ===============================
platform linux -- Python 3.8.9, pytest-6.2.3, py-1.10.0, pluggy-0.13.1
rootdir: /home/tkloczko/rpmbuild/BUILD/nox-2020.12.31
plugins: forked-1.3.0, shutil-1.7.0, virtualenv-1.7.0, asyncio-0.14.0, expect-1.1.0, cov-2.11.1, mock-3.5.1, httpbin-1.0.0, xdist-2.2.1, flake8-1.0.7, timeout-1.4.2, betamax-0.8.1, pyfakefs-4.4.0, freezegun-0.4.2, flaky-3.7.0, cases-3.4.6, hypothesis-6.10.1, case-1.5.3, isort-1.3.0, aspectlib-1.5.2
collected 77 items

tests/test_sessions.py .............................................................................[100%]

====================== 77 passed in 1.53s ================================

contexter module is not maintained since 2017.

Tested using pytest

$ /usr/bin/python3 -Bm pytest -ra tests/test_sessions.py
===================== test session starts ===============================
platform linux -- Python 3.8.9, pytest-6.2.3, py-1.10.0, pluggy-0.13.1
rootdir: /home/tkloczko/rpmbuild/BUILD/nox-2020.12.31
plugins: forked-1.3.0, shutil-1.7.0, virtualenv-1.7.0, asyncio-0.14.0, expect-1.1.0, cov-2.11.1, mock-3.5.1, httpbin-1.0.0, xdist-2.2.1, flake8-1.0.7, timeout-1.4.2, betamax-0.8.1, pyfakefs-4.4.0, freezegun-0.4.2, flaky-3.7.0, cases-3.4.6, hypothesis-6.10.1, case-1.5.3, isort-1.3.0, aspectlib-1.5.2
collected 77 items

tests/test_sessions.py .............................................................................[100%]

====================== 77 passed in 1.53s ================================
Copy link
Collaborator

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Hi @kloczek thank you for reporting this, as well as providing a PR with a fix!

AFAICS we were only using contexter for its Python 2 support, not for any special features. So your approach of using contextlib instead should indeed work fine.

Please see my inline comments with suggestions how to fix the CI checks. As contextlib is not a PyPI package but a standard library, replacing all occurrences of contexter by contextlib does not work.

@@ -332,7 +332,7 @@ incompatible versions of packages already installed with conda.

.. code-block:: python

session.install("contexter", "--no-deps")
session.install("contextlib", "--no-deps")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest to leave this unchanged. This is an example in the documentation for an arbitrary third-party library.

Also this does not work, contextlib cannot be installed from PyPI. Nor does it need to be, it's part of the standard library.

@@ -58,7 +58,7 @@ def conda_tests(session):
session.conda_install(
"--file", "requirements-conda-test.txt", "--channel", "conda-forge"
)
session.install("contexter", "--no-deps")
session.install("contextlib", "--no-deps")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should be removed. There's no need to install contextlib, because it's part of the standard library.

Suggested change
session.install("contextlib", "--no-deps")

@@ -1,7 +1,7 @@
flask
pytest
pytest-cov
contexter
contextlib
Copy link
Collaborator

Choose a reason for hiding this comment

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

This requirement should be removed, see above.

Suggested change
contextlib

@@ -17,7 +17,7 @@
from pathlib import Path
from unittest import mock

import contexter
import contextlib
Copy link
Collaborator

Choose a reason for hiding this comment

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

This import needs to be moved up to the start of the imports section, because it is a standard library import, not a third-party import.

This is what caused the lint check to fail in CI.

@kloczek
Copy link
Author

kloczek commented May 3, 2021

AFAICS we were only using contexter for its Python 2 support, not for any special features. So your approach of using contextlib instead should indeed work fine.

Python 2 have been long time ago EOSed so IMO it is time to remove on the master all python 2.x suppoert :)

@henryiii
Copy link
Collaborator

henryiii commented May 4, 2021

contextlib2 supports Python 2 and has a final stable Python 2 version (2019). I'd import from contextlib2 if sys.version_info < (3,)

@kloczek
Copy link
Author

kloczek commented May 9, 2021

As python 2.x is now EOSed keeping still support for that version of python today is IMO a bit odd.

@henryiii
Copy link
Collaborator

henryiii commented May 9, 2021

That's up to the authors. Packages designed to run many versions of Python (virtualenv, tox, and of course nox) may want to keep Python 2 longer than normal packages. This could also be seen as a reason to use tox over nox.

Not saying Python 2 should be kept here. And using an older nox is always a possibility for Python 2 users. But am saying it is the authors decision.

@henryiii
Copy link
Collaborator

henryiii commented May 9, 2021

Also, and, personally, dropping something because of a test is _ really irritating. Maybe it's just me. But I changed 5K LOC to go from GTest to Catch2 gust because GTest was going to force me to drop old compilers. Now for a feature, like full static typing, etc, or simpler code, sure.

@kloczek
Copy link
Author

kloczek commented May 9, 2021

Also, and, personally, dropping something because of a test is _ really irritating. Maybe it's just me. But I changed 5K LOC to go from GTest to Catch2 gust because GTest was going to force me to drop old compilers. Now for a feature, like full static typing, etc, or simpler code, sure.

Don't get me wrong but it inot about test. As at the moment 2.x is EOSed do remove python 2.x code on the master is kind of against KISS principle.
All what is needed IMO is just document which one last version is for python 2.x and remove all python 2.x traces on master. Eventually if you still want to provide python 2.x support source code could be always branched at exact point list of changes and some fixes with updated release can be done.

Simple it is IMO about keep code on th emaster as clean as it is only possible.
Only this and nothing more😊

@cjolowicz
Copy link
Collaborator

@kloczek Are you planning to address the points raised above?

@henryiii Nox has dropped Python 2 support a while ago, so I don't see a reason to pull in a third-party dependency here. To be clear, we're talking about the Python version that Nox itself runs under.

@henryiii
Copy link
Collaborator

I saw it in a nox file and assumed it was for testing a python 2 session. I think I was wrong, though.

@kloczek
Copy link
Author

kloczek commented May 23, 2021

Sorry have no time now :(
Feel free to close this PR and implement it how you like.

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

Successfully merging this pull request may close these issues.

None yet

3 participants