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

Drop readline workaround introduced in #1281 #8848

Merged

Conversation

nicoddemus
Copy link
Member

Fix #8733
Closes #8847

  • Also add types-atomicwrites as extras to mypy pre-commit hook (Windows)

@nicoddemus
Copy link
Member Author

As commented in #8847 (comment):

Not sure about this. As evident by this issue people still seem to have pyreadline installed, and depending on how obscure the behavior with pyreadline and --pdb is, I'd rather have them see a deprecation warning (making them realize they should probably uninstall pyreadline) than having behavior which is probably much harder to track down.

Fair enough, what do you propose? A warning in Python <=3.9, and skipping it altogether in Python 3.10?

@The-Compiler
Copy link
Member

Fair enough, what do you propose? A warning in Python <=3.9, and skipping it altogether in Python 3.10?

Depends on what happens when pdb is invoked on Windows 3.10 with pyreadline installed. If our workaround breaks things (via pyreadline) then skip it; if things are broken anyways (because pdb imports readline), then we might as well keep it as-is.

To me this seems similar to what'd happen if someone created, say, a custom pathlib.py shadowing the stdlib one - it's not a situation we should have to consider. But if the workaround breaks things which would work otherwise, then let's get rid of it.

@nicoddemus
Copy link
Member Author

Depends on what happens when pdb is invoked on Windows 3.10 with pyreadline installed.

IIUC it breaks in Python 3.10 with:

  File "d:\a\gstore\gstore\.venv\lib\site-packages\pyreadline\py3k_compat.py", line 8, in callable
    return isinstance(x, collections.Callable)
AttributeError: module 'collections' has no attribute 'Callable'

(As per #8733 (comment)).

But if the workaround breaks things which would work otherwise, then let's get rid of it.

That was the rationale; and given the workaround was done in 2015 and it was not clear if it was reported in Python 2, it might be the case that dropping it completely is OK.

What do you think?

@The-Compiler
Copy link
Member

Hm, I see - so with Python 3.10 we'd break pytest entirely if pyreadline is installed, and without the workaround, it wouldn't break at all (or only if --pdb is used, because I assume that will do import readline in pdb?).

How does --pdb break without the workaround when pyreadline is installed?

Don't really have a strong opinion on this either way, though.

@nicoddemus
Copy link
Member Author

How does --pdb break without the workaround when pyreadline is installed?

That's described in #1281, however I don't recall experiencing the problem myself.

@RonnyPfannschmidt
Copy link
Member

Let's disable the workaround on py 3.10 and schedule the removal to pytest 8

@nicoddemus
Copy link
Member Author

Let's disable the workaround on py 3.10 and schedule the removal to pytest 8

Do we issue a warning for Python <3.9? If so, what should the warning say?

@RonnyPfannschmidt
Copy link
Member

If pyreadline is involved we should give a hint, im not sure about the exact message

@nicoddemus
Copy link
Member Author

nicoddemus commented Jul 2, 2021

Currently pyreadline is involved indirectly:

Traceback (most recent call last):
  ...
  File "d:\a\gstore\gstore\.venv\lib\site-packages\_pytest\capture.py", line 90, in _readline_workaround
    import readline  # noqa: F401
  File "d:\a\gstore\gstore\.venv\lib\site-packages\readline.py", line 34, in <module>
    rl = Readline()
  File "d:\a\gstore\gstore\.venv\lib\site-packages\pyreadline\rlmain.py", line 422, in __init__
    BaseReadline.__init__(self)
  File "d:\a\gstore\gstore\.venv\lib\site-packages\pyreadline\rlmain.py", line 62, in __init__
    mode.init_editing_mode(None)
  File "d:\a\gstore\gstore\.venv\lib\site-packages\pyreadline\modes\emacs.py", line 633, in init_editing_mode
    self._bind_key('space',       self.self_insert)
  File "d:\a\gstore\gstore\.venv\lib\site-packages\pyreadline\modes\basemode.py", line 162, in _bind_key
    if not callable(func):
  File "d:\a\gstore\gstore\.venv\lib\site-packages\pyreadline\py3k_compat.py", line 8, in callable
    return isinstance(x, collections.Callable)
AttributeError: module 'collections' has no attribute 'Callable'

We import readline, which ends up importing pyreadline as result.

@nicoddemus
Copy link
Member Author

Hi folks, sorry for the delay here.

Not sure how to proceed... I agree about dropping this for Python 3.10, however what should the warning say for Python<3.9 users? Note that we can't reasonably "warn" or suggest about anything, if we import readline and that succeeds, I can't really think of a good warning message. Something like this comes to mind:

We have a workaround implemented in issue XXX for the readline module, and that's disabled in Python 3.10+. Take a look and comment on the issue if you believe the workaround needs to stay in some form.

But that doesn't seems really helpful or actionable.

@nicoddemus
Copy link
Member Author

Gentle ping here. How should we proceed?

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Per #8847 (comment), this PR LGTM.

@The-Compiler
Copy link
Member

Given that this is long done, approved and a breaking change, let's sneak it into 7.0.0.

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.

Pytest warning on readline Pytest run fails under Python 3.10.0-beta.2 on Windows
4 participants