Get rid of the "py" module dependency #77
Conversation
| def _getdimensions(): | ||
|
|
||
| if sys.version_info >= (3, 3, 0): | ||
| import shutil |
There was a problem hiding this comment.
Could we move the imports outside of the function together with the other imports? You can still use the if condition on the imports. Having the import machinery invoked in functions is not nice, it makes it much harder to know when what is happening and can result in weird locking issues and hard to debug circular import issues etc.
| # pass to fallback below | ||
| pass | ||
|
|
||
| if width == 0: |
There was a problem hiding this comment.
Why have a sentinel of 0? You can just directly put the block of this if condition in the except block which I think is better style.
| except (KeyboardInterrupt, SystemExit, MemoryError, GeneratorExit): | ||
| raise | ||
| except: |
There was a problem hiding this comment.
Any reason to do this rather than the more traditional single except clause?
except Exception:
pass
| @@ -433,6 +432,43 @@ def dump_stacks(): | |||
| write("".join(traceback.format_stack(frame))) | |||
|
|
|||
|
|
|||
| def _getdimensions(): | |||
There was a problem hiding this comment.
I think this is more consistently named get_terminal_dimensions() given the other name (especially the consistency of when using underscores between words, the names are more bikesheddy and I don't mind as much).
Also no need to prefix either of these two functions with underscores here. This module does not have a public API and we haven't been doing this for other functions (uh, lol, that's not true, but probably should have been true)
|
Actually, I made a crude hack. I picked up these functions "as is" from the "py" codebase. Having a lot on my plate I can't dig deeper, but I hope I'll find some time to investigate and clean that hunk according to your demands. |
|
On Sat 24 Oct 2020 at 09:33 -0700, zentarim wrote:
Do I need to maintain backward compatibility with Python2.7?
Hmm, given that pytest itself seems to have dropped this perhaps it's
fine to remove python 2.7 support. So I won't reject matching the
supported versions to pytest's own set of supported versions. Even if
it makes me a little sad.
|
|
Btw, do you know what pytest itself uses for this? I suspect the
original intention was to use the same mechanism as pytest uses. If
pytest migrated away from py and does this some other way the code here
should probably do the same as pytest itself.
|
| @@ -433,6 +432,43 @@ def dump_stacks(): | |||
| write("".join(traceback.format_stack(frame))) | |||
|
|
|||
|
|
|||
| def _getdimensions(): | |||
|
|
|||
| if sys.version_info >= (3, 3, 0): | |||
There was a problem hiding this comment.
pytest-timeout only supports python 3.6+ now so this condition isn't needed
|
Can this be accomplished now? |
|
@flub I can, but I prefer to get confirmation from another maintainer before closing PRs. |
The "py" module is now in maintenance mode and should not be used in new code. Moreover, only one method from the "py" module is used in the pytest-timeout. I suppose it is better to try to remove this dependency.