click icon indicating copy to clipboard operation
click copied to clipboard

click.confirm fails when piping input

Open holmescharles opened this issue 6 years ago • 5 comments

I string a lot of my programs together through piping, but some of them work best with confirmation prompting. However, if I pipe program A's output into program B and B asks for confirmation, click.confirm will not be able to get keyboard input since stdin is the pipe, not the terminal.

It seems that a work around would be to use click.getchar, which uses the terminal no matter what, but this would mean a few lines of code + an if statement to get the the same functionality.

Would it be possible to get click.confirm to always get input from the terminal? Or at least include an option for that?

holmescharles avatar Aug 15 '19 19:08 holmescharles

Feel free to make a PR for this if you have an implementation in mind.

In general saving a few lines in user code is not a strong reason for a feature request, but if the implementation is low impact it could get added.

jcrotts avatar Aug 16 '19 18:08 jcrotts

I was also trying to implement the same functionality. For example

import click

def get_name(ctx, param, value):
    if not value and not click.get_text_stream('stdin').isatty():
        return click.get_text_stream('stdin').read().strip()
    else:
        return value


@click.command()
@click.argument('name', callback=get_name, required=False)
def say_hello(name):
    #click.confirm('Do you want to continue?', abort=True, default=True)
    click.echo('Hello {}'.format(name))



if __name__ == "__main__":
    say_hello()

Normal test case

λ python test_pipe.py click
Hello click

Missing argument

λ python test_pipe.py
Hello None

Pipelined argument

λ echo click | python test_pipe.py
Hello click

Now when confirmation logic added

@click.command()
@click.argument('name', callback=get_name, required=False)
def say_hello(name):
    click.confirm('Do you want to continue?', abort=True, default=True)
    click.echo('Hello {}'.format(name))

Normal test case

λ python test_pipe.py click
Do you want to continue? [Y/n]: y
Hello click

Missing argument

λ python test_pipe.py
Do you want to continue? [Y/n]: y
Hello None

Pipelined argument

λ echo click | python test_pipe.py
Do you want to continue? [Y/n]: Aborted!

I have then added the following modifications

At termui.py i have added the following method

def _get_user_confirmation():
    ret = ''
    while 1:
        c = getchar()
        if c == '\r':
            break
        elif c in ('\x08', '\x7f') and len(ret) != 0:
            echo("\b \b", nl=False)
            ret = ret[:-1]
        else:
            ret += c
            echo(c, nl=False)
    echo('', nl=True)
    return ret

and at the confirm method

    prompt = _build_prompt(text, prompt_suffix, show_default,
                           default and 'Y/n' or 'y/N')
    while 1:
        try:
            # Write the prompt separately so that we get nice
            # coloring through colorama on Windows
            echo(prompt, nl=False, err=err)
            #value = visible_prompt_func('').lower().strip()
            value = _get_user_confirmation().lower().strip()
        except (KeyboardInterrupt, EOFError):
            ...

Pipelined argument

λ echo click | python test_pipe.py
Do you want to continue? [Y/n]: y
Hello click

λ echo click | python test_pipe.py
Do you want to continue? [Y/n]: n
Aborted!

I wasn't able to get the unit-tests running (test_util.py) as the test_prompt method expects an input from the terminal. Also with this modification we still need to define the argument as a non required one

I am trying to do a my first contribution to click, hopefully i can get this working

9ronoob avatar Aug 19 '19 12:08 9ronoob

Seems like either Click's confirmation should be disabled when reading input from stdin, or the program doing the piping should also be outputting answers to the confirmations. Changing inputs to read from one source for some and a different way for others seems confusing.

davidism avatar Aug 21 '19 19:08 davidism

I have gone through another approach. Now writing a code like this

import click

@click.command()
@click.argument('name', required=True)
@click.option('--m', required=True)
def say_hello(name, m):
    click.confirm('Do you want to continue?', abort=True, default=True)
    click.echo('Hello {}'.format(name)  + m)


if __name__ == "__main__":
    say_hello()

Shall work as expected

Normal test case

λ python test_pipe.py world --m 1
Do you want to continue? [Y/n]: y
Hello world1

Missing argument

λ python test_pipe.py
Usage: test_pipe.py [OPTIONS] NAME
Try "test_pipe.py --help" for help.

Error: Missing argument "NAME".

Pipelined argument

 λ echo world | python test_pipe.py --m 1
Do you want to continue? [Y/n]: y
Hello world1

And without confirmation

@click.command()
@click.argument('name', required=True)
@click.option('--m', required=True)
def say_hello(name, m):
    #click.confirm('Do you want to continue?', abort=True, default=True)
    click.echo('Hello {}'.format(name)  + m)

Normal test case

λ python test_pipe.py world --m 1
Hello world1

Missing argument

λ python test_pipe.py
Usage: test_pipe.py [OPTIONS] NAME
Try "test_pipe.py --help" for help.

Error: Missing argument "NAME".

Pipelined argument

 λ echo world | python test_pipe.py --m 1
Hello world1

Now i no longer need to define a callback for getting the arguments when passed through the stdin pipe.

A test-case that can expose this bug.

    @click.command()
    @click.argument("arg1", required=True)
    def test_args_pipe_confirm(arg1):
        if click.confirm('Foo', default=True):
            click.echo(arg1)
        else:
            click.echo('no :(')

    result = runner.invoke(test_args_confirm, input="test_age\ny\n")
    assert not result.exception
    assert result.output == 'Foo [Y/n]: y\ntest_age\n'

I have submitted these changes into a pull request at https://github.com/pallets/click/pull/1372

Looking forward to your feedback

9ronoob avatar Aug 21 '19 19:08 9ronoob

I'm interested in opening a new PR for this based on @9ronoob 's work in #1372

@davidism Does the proposed solution above address any concerns?

jonbiemond avatar Apr 23 '24 17:04 jonbiemond