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

Fix issues with CliRunner's echo_stdin #1820

Merged
merged 2 commits into from
Apr 14, 2021

Conversation

saebischer
Copy link
Contributor

Fixes #1101 and some related issues with CliRunner(echo_stdin=True).

As raised in #1101, with Python 2 any prompted input appears twice in the output recorded by CliRunner when using echo_stdin=True. It appears to work with Python 3 because EchoingStdin does not always echo. For example, using input instead of click.prompt:

@click.command()
def test():
    foo = input("Foo: ")
    click.echo("foo={}".format(foo))

runner = CliRunner(echo_stdin=True)
result = runner.invoke(test, input="bar\n")
print(result.output)

Expected output:

Foo: bar
foo=bar

Actual output:

Foo: foo=bar

Environment: Click 7.1.2, Python 3.7.7

Here the problem was that TextIOWrapper tries to call read1 on the underlying input stream, which wasn't being overridden in EchoingStdin. After adding an echoing read1, both Python 2 and Python 3 double-echo with click.prompt.

I've modified visible_input not to echo when echo_stdin=True. To get consistent behaviour for hidden prompts (which should not echo even if echo_stdin=True) I also added a way to temporarily turn off echoing inside hidden_input. This is also used in _getchar if it gets passed echo=False.

Finally, there was one more subtle problem in Python 3. TextIOWrapper provides a buffered stream that reads a large chunk from the input before parsing out individual lines. When the underlying input stream is EchoingStdin, this causes all input (up to the chunk size) to be echoed at once. Here's another example:

@click.command()
def test():
    click.prompt("a")
    click.prompt("b")

runner = CliRunner(echo_stdin=True)
result = runner.invoke(test, input="foo\nbar\n")
print(result.output)

Expected output:

a: foo
b: bar

Actual output:

a: foo
bar
b: 

Environment: commit 21cde2f (after the previously mentioned issues are fixed), Python 3.7.7

The only solution I found for this was to set the _CHUNK_SIZE attribute of TextIOWrapper, which is not documented (I found it here) and I guess that means it might not be compatible with all Python implementations. It would be great if someone knows a cleaner approach, otherwise I hope it's okay to contribute something like this.


Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code. (n/a)
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs. (n/a)
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@saebischer
Copy link
Contributor Author

src/click/utils.py:129:9: B014 Redundant exception types in `except (IOError, OSError) as e:`.  Write `except OSError as e:`, which catches exactly the same exceptions.

Not sure about this one as I haven't made any changes to src/click/utils.py. I can see on master that it has been changed to just OSError as suggested. Should I do the same here even though it's unrelated to the main issue? New to this so guidance appreciated!

@davidism
Copy link
Member

You should target this pr at master, not 7.x.

@saebischer saebischer changed the base branch from 7.x to master March 23, 2021 20:29
@saebischer
Copy link
Contributor Author

Thanks @davidism, I've updated the base branch. I thought since this is a bugfix the contributing guide said to target the latest maintenance branch.

@davidism
Copy link
Member

You need to rebase, in addition to changing it in github.

@saebischer saebischer force-pushed the fix-clirunner-echo_stdin branch from 38e5c31 to d1cca29 Compare March 23, 2021 20:56
@davidism davidism added this to the 8.0.0 milestone Apr 13, 2021
saebischer and others added 2 commits April 13, 2021 13:28
echo calls to read1
pause echo when using hidden input prompt and getchar
don't buffer reads to avoid echoing early
pause echo_stdin unconditionally, allowing functions to echo as normal
this seems to work better with the readline "echo empty string" fix
@davidism davidism force-pushed the fix-clirunner-echo_stdin branch from d1cca29 to f6f8976 Compare April 14, 2021 14:49
@davidism
Copy link
Member

#1836 changed how the prompt was echoed to work around a readline issue. Splitting echoing the prompt from the space after it seems to mess up the tests you wrote. I changed to unconditionally disabling echo_stdin behavior during the visible_input, hidden_input, and _getchar functions, and this seems to have fixed it. Used a context decorator instead of managing the flag manually.

@davidism davidism merged commit 30e7f76 into pallets:master Apr 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CliRunner prompt input echos twice on Python 2
2 participants