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

Make the isatty method of OutStream return True #683

Merged
merged 7 commits into from
Jun 9, 2021

Conversation

peendebak
Copy link

@peendebak peendebak commented Jun 4, 2021

The ipykernel.iostream.OutStream represents an interactive stream and should there have a isatty method returning True.
Also see #268, #680, Textualize/rich#1221

Fixes #680
Fixes #268

@blink1073 blink1073 added this to the 6.0 milestone Jun 4, 2021
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me, thank you! I'll leave this open until Monday in case another maintainer knows of a reason not to enable this.

@real-yfprojects
Copy link

You should add Closes #680 and closes #268 .
Also thank you for creating the pull request. I didn't know this was the faster way to go.

@ccordoba12
Copy link
Member

Although this would be quite useful to support Rich and Colorama in Spyder and the notebook, I don't know if it could have side effects on IPython's display system. I mean, there should be a reason why the OutStream is expected to behave as a TextIO object.

@minrk, @takluyver, @Carreau, what do you think?

@Carreau
Copy link
Member

Carreau commented Jun 7, 2021

I think this is a bad idea. A lot of things assume that isatty() == True, mean you can put the terminal in raw mode and/or support cursor motion up/down to redraw previous lines.

Things like pagers ( less, more) and many others will start to misbehave if you set tty to true.

@ccordoba12
Copy link
Member

Thanks for the input @Carreau! What if we make this change configurable then? That way, users that know what they are doing could enable this functionality for Rich and Colorama. In Spyder we could also expose this option in our Preferences system and inform users to activate it if we detect they are working with those libraries.

@dsblank
Copy link
Contributor

dsblank commented Jun 7, 2021

It is too bad that there is a single flag that signals if an output can handle simple line control (like backspace), colors, and full cursor movements. We have a commercial product and almost 100% of the non-interactive output works well when istty() returns True.

@Carreau
Copy link
Member

Carreau commented Jun 7, 2021 via email

@dsblank
Copy link
Contributor

dsblank commented Jun 7, 2021

No objections to making in configurable

Agreed, that sounds like a good approach!

@peendebak
Copy link
Author

@dsblank @Carreau @blink1073 @ccordoba12 I will make the option configurable, so that projects using the ipykernel can pick whichever mode they want.

The only remaining question is what the default value will be for this option. The safe option would be False. On the other hand, True might be the better option for a larger number of cases.

@ccordoba12
Copy link
Member

The only remaining question is what the default value will be for this option. The safe option would be False. On the other hand, True might be the better option for a larger number of cases.

It needs to be False to be on the safe side and not to change the current behavior.

@peendebak
Copy link
Author

@blink1073 The additional commits are under another github account. Could you approve the CI?

@blink1073
Copy link
Contributor

Could you approve the CI?

Done!

@blink1073
Copy link
Contributor

Looks like tests need updating again

@blink1073
Copy link
Contributor

@peendebak
Copy link
Author

The linter flagged one thing: https://github.com/ipython/ipykernel/pull/683/checks?check_run_id=2777466679#step:8:21

@blink1073 Thanks, I fixed the formatting.

@blink1073 blink1073 merged commit 2ec4cbf into ipython:master Jun 9, 2021
@blink1073
Copy link
Contributor

Thanks for the contribution @peendebak!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ipykernel.iostream.OutStream.isatty() should be True. OutStream.isatty
7 participants