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

Allow default Console parameters to be configured with environment variables #2122

Closed
wants to merge 1 commit into from

Conversation

joouha
Copy link

@joouha joouha commented Mar 27, 2022

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 allows the default values of the Console parameters force_jupyter, force_terminal & force_interactive to be overridden by setting the environment variables RICH_FORCE_JUPYTER, RICH_FORCE_TERMINAL & RICH_FORCE_INTERACTIVE.

Fixes issue #2119

@willmcgugan
Copy link
Collaborator

I'm not keen on adding env vars that aren't already a standard. As a library, Rich shouldn't impose its own env vars on to applications. It should be the application author that decides how to handle env vars.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #2122 (c012f3d) into master (6dfb81d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2122   +/-   ##
=======================================
  Coverage   99.48%   99.48%           
=======================================
  Files          72       72           
  Lines        7334     7348   +14     
=======================================
+ Hits         7296     7310   +14     
  Misses         38       38           
Flag Coverage Δ
unittests 99.48% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
rich/__init__.py 100.00% <100.00%> (ø)
rich/console.py 99.15% <100.00%> (+0.01%) ⬆️
rich/repr.py 100.00% <100.00%> (ø)
rich/traceback.py 99.55% <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 fbc831e...c012f3d. Read the comment docs.

@joouha
Copy link
Author

joouha commented Mar 27, 2022

Fair enough.

I see that Spyder had the same issue I am experiencing here:

#1221 (comment)

(In the link above you suggested adding a generic environment variable for exactly this purpose).

Spyder managed to work around the issue by creating their own kernel wrapper around ipykernel, which allows them to set isatty for stdout to True, and change ipython.__class__.__name__, meaning that the default values for Console.is_jupyter becomes False and Console.is_terminal becomes True (this is what I am trying to achieve).

However, creating my own kernel isn't really an option for me with euporie, as users need to be able to run any kernel of their choosing.

I could override the kernel's configuration when it is launched to add hooks that make the required changes to the kernel. But, the kernel does not necessarily run in the same python environment or interpreter, so this would require users to install an extra package which contains the hooks (not ideal).


My other idea is that rich could add a metadata dictionary to the output of JupyterRenderable._repr_mimebundle_ which would allow me to identify that the output comes from rich and therefore I probably want to choose to render the text/plain output over the text/html one. (Usually text/html trumps text/plain, but not for rich's output).

I suppose that I could simply search the text/plain output for terminal escape sequences and make a decision about what to render based on that.

Do you have any other ideas?

@joouha
Copy link
Author

joouha commented Mar 28, 2022

I've done some testing, and rendering the text/plain output of JupyterRenderable._repr_mimebundle_ from rich doesn't help. Since euporie doesn't support jupyter-widgets, the clear_output call made when a "live" rich renderable refreshes causes the entire cell output to clear. This means that you wouldn't be able to use rich.progress.track and display other output in the same cell, for example. (This is similar to what is going on in #2112).

As a library, Rich shouldn't impose its own env vars on to applications. It should be the application author that decides how to handle env vars.

I'm not sure how this should apply when rich is being use interactively in a Jupyter notebook. The author of a Jupyter notebook is the one writing the code which calls rich, but I don't think they would expect to have to change their code to reconfigure rich's Console depending on which notebook editor they are using.

@willmcgugan
Copy link
Collaborator

@joouha I think I need to understand your use case a bit more. Would you be free for a video chat, next week perhaps?

@joouha
Copy link
Author

joouha commented Apr 4, 2022

@willmcgugan Yes, I can do a video call. My e-mail is in my profile if you'd like to set something up.

@Commandcracker
Copy link

It would be nice to see this Implemented!
I can see many use cases for a force_terminal env var.

Examples:

  • Some Terminals support colors but rich isn't detecting it and editing every package to force_terminal isn't worth the time.

  • in GitHub actions force_terminal is required to enable color. Setting an env var in a Dockerfile would be an easy fix. also i don't know how to tell pip to force_terminal when i run it from shell.

force_terminal=False
Screenshot_20220528_221839

force_terminal=True
Screenshot_20220528_221912

  • Big packages like twine also have some issues not having an env var for it. wich can be seen here

@willmcgugan
Copy link
Collaborator

Closing. Assumed stale.

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.

None yet

4 participants