pallets / click

Python composable command line interface toolkit
https://click.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
15.76k stars 1.4k forks source link

`CliRunner.isolation()` incompatible return type with usage in `invoke()` #1773

Closed ezdac closed 3 years ago

ezdac commented 3 years ago

I did not encounter a problem due to this, but I found inconsistencies in the code.

Actual Behavior

The tuple yielded by the CliRunner.isolation() contextmanager is incompatible with the usage in the CliRunner.invoke() method. In the latter, a BytesIO object is assumed, while the actual return value is a boolean (literal).

https://github.com/pallets/click/blob/fbe33bac440d0edef50fc980fb638d2dbbf96b5e/src/click/testing.py#L265

https://github.com/pallets/click/blob/fbe33bac440d0edef50fc980fb638d2dbbf96b5e/src/click/testing.py#L370

I guess the intent was to only return the BytesIO when self.mix_stderr is True, So I'd rather bind bytes_error as None before and just pass it

bytes_error = None
if self.mix_stderr:
    sys.stderr = sys.stdout
else:
    bytes_error = io.BytesIO()
    ...
...
yield (bytes_output, bytes_error)

But I'm unsure of what is expected behaviour here, otherwise I would simply fix it.


### Environment
* Click version: 7.0
davidism commented 3 years ago

Sure, that change makes sense. On the other hand, I don't think it's particularly a problem to return BytesIO, False.

thisisamardeep commented 3 years ago

i am picking this up.Will get back.

Saif807380 commented 3 years ago

Hey, I'm a fellow from MLH. I'd like to work on this issue.