Skip to content

Conversation

@ccordoba12
Copy link

@ccordoba12 ccordoba12 commented May 8, 2021

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

  • This detects Spyder as a Jupyter environment.
  • Spyder uses qtconsole for its main console, but with a lot of customizations on top of it to connect it to its other panes (including a subclass of ZMQInteractiveShell).
  • That's why it was not recognized by Rich as a Jupyter environment.
  • This is a much simpler and less invasive change than PR fix color output in spyder console #1218.

- Spyder uses qtconsole for its main console, but with a lot of
customizations on top of it to connect it to its other panes
(including a subclass of ZMQInteractiveShell).
- That's why it was not recognized by Rich as a Jupyter
environment.
@ccordoba12
Copy link
Author

ccordoba12 commented May 8, 2021

I'm the Spyder maintainer, so in case you consider this change is not good enough, please let me know what else I could do to fix the situation for our users.

@codecov
Copy link

codecov bot commented May 9, 2021

Codecov Report

Merging #1221 (dd783aa) into master (869f5ba) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1221   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          69       69           
  Lines        6379     6379           
=======================================
  Hits         6368     6368           
  Misses         11       11           
Flag Coverage Δ
unittests 99.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rich/console.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4debb9...dd783aa. Read the comment docs.

@willmcgugan
Copy link
Member

I think there are two issue which break Rich in Spyder. The first is that Spyder wraps stdout with a file object where isatty returns False. Rich uses that method to disable color when not writing to a terminal. I see that issue was raised on the Spyder repo.

The second may be colorama which is a bit of a hack on Windows to get color on the classic terminal. It works fine for a terminal running with conhost.exe but not an emulated terminal as I guess you have in Spyder. Unfortunately, I don't have a good solution for this issue. You could run colorama.deinit but that wouldn't work out-of-the box.

An env var may be best workaround at the moment, but I wouldn't approve of a specific env var for every IDE or terminal window. Possibly we could come up with a generic env var.

Happy to brainstorm ideas, because of course I would want Spyder users to be able to enjoy Rich.

That said, if this change works for you, I could merge that. But bear in mind that Jupyter support may not be as good as a regular terminal. There's no way in Jupyter to get the width of the terminal which is needed for things like Panels and Tables.

@eendebakpt
Copy link

For colorama to work in Spyder one can pass the option strip=False during initialization. For the default value strip=None colorama tries to determine the appropriate value here:

https://github.com/tartley/colorama/blob/7a85efbc6d5b59665badb50b953d12390047b5f8/colorama/ansitowin32.py#L92

...
        conversion_supported = on_windows and winapi_test()

        # should we strip ANSI sequences from our output?
        if strip is None:
            strip = conversion_supported or (not self.stream.closed and not self.stream.isatty())
        self.strip = strip
... 

Note the check with self.stream.isatty(). I suspect this causes colorama to use strip=True, which results in colored being removed, similar to what happens in rich.

@ccordoba12
Copy link
Author

ccordoba12 commented May 9, 2021

An env var may be best workaround at the moment, but I wouldn't approve of a specific env var for every IDE or terminal window. Possibly we could come up with a generic env var.

Agreed. That would very easily allow you to control when to turn on/off certain features according to what an environment (notebook, editor, IDE) declares.

If you can provide the (generic) name for it in a PR here, I could create a PR to set it in the appropriate Jupyter repo so that both Spyder and Jupyter use it out of the box.

That said, if this change works for you, I could merge that

It works in the sense that it gives Spyder the same support Jupyter has right now. But I'm fine if you prefer to wait until the env var solution is in place.

@ccordoba12
Copy link
Author

For colorama to work in Spyder one can pass the option strip=False during initialization

We could patch colorama in our kernel (i.e. spyder-kernels) to make that work.

But I have one question about it: Does colorama work in Spyder without issues on Linux and Mac?

@peendebak
Copy link

@ccordoba12 For Linux colorama works with init(strip=False), but not with init(). Mac I do not have access to

@willmcgugan
Copy link
Member

@ccordoba12 colorama shouldn't have any effect on other platforms. From the docs:

On other platforms, calling init() has no effect (unless you request other optional functionality; see “Init Keyword Args”, below). By design, this permits applications to call init() unconditionally on all platforms, after which ANSI output should just work.

By the looks of things, I can set strip=False globally. If you make sure that isatty() returns True for your wrapped stdout, then I think that will give you Rich support. If that works for you, I can add it to the next version.

A related is that Rich is unable to detect the terminal size, so it will be stuck at 80 columns. This is because os.get_terminal_size is throwing an error on Spyder.

@eendebakpt
Copy link

@ccordoba12 Would it be possible for you to make isatty() return True (for the appropriate situations)?

@willmcgugan
Copy link
Member

THis should be fixed in Spider could return True from its proxy isatty()

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.

4 participants