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

Added echo, echo_via_pager #29

Closed
wants to merge 9 commits into from
Closed

Conversation

RhetTbull
Copy link

Adds echo() and echo_via_pager() commands that work just like the click variants but support rich markup.

@ewels
Copy link
Owner

ewels commented Mar 1, 2022

Thanks for this! Will review soon but two speed questions:

  • What's the use case for continuing to use these if the developer is already pulling the rich library (which has native functions equivalent to these calls)?
  • What do you think about making rich markup / markdown opt-in via the globals that we're already using for the CLI help texts?

@RhetTbull
Copy link
Author

RhetTbull commented Mar 1, 2022

  • What's the use case for continuing to use these if the developer is already pulling the rich library (which has native functions equivalent to these calls)?

For me, it's to drop in replace for code that's using click.echo/echo_via_pager. click.echo in particular does a number of things to "play nice" with click apps / progress bars / auto-flush, etc. For me, if I'm pulling rich-click in, it would be nice for it to "richify" all the click things including echo, etc. Using these as drop-in replacements lets the developer slowly "richify" an existing code base.

Another use case (and the reason I originally wrote these) is that rich + click.testing.CliRunner + pytest doesn't work. The issue is closed in rich repo but the problem still exists. If you use rich print in a click app and use pytest + the click CliRunner to test, the rich output doesn't get captured. This code lets you use click.echo which does work with pytest and the test runner but get rich formatting.

  • What do you think about making rich markup / markdown opt-in via the globals that we're already using for the CLI help texts?

I'm already working on that...didn't realize how you were using those globals before I sent the PR. I'm working on an update that will work with Markdown and honor the globals you're using.

EDIT: added another commit which does this.

@ewels
Copy link
Owner

ewels commented Mar 2, 2022

Ok. If you don't mind, I'm going to come back to this after I've looked into #25 (setting up unit tests for this repo).

My gut feeling is that I don't want to expand the scope of the package any more than I have to and that I would prefer developers to use native rich functions directly (less chance of error, less maintenance here). If you're going to start editing strings to use rich markup codes, it's not much more code to use the rich function calls as well.

The difficulty with testing is more pertinent, but I wonder if we can get around that in a different way seeing as rich-click doesn't use click.echo at all (or rich.print, only console.print). As we are purely using rich console objects, I hope that this might mean that we can sidestep the problems in the issue you mentioned. However, this is pure conjecture and it's fairly likely that I'll hit exactly the problems you mention. In which case, I'll come back and merge this PR to get stuff to work 😅

Phil

@RhetTbull
Copy link
Author

No worries -- I'll keep using these "rich-ified" versions in my own code. The package name "rich-click" made me think this is an appropriate place for these functions as that implies more than just "rich help text". Cheers!

@ewels
Copy link
Owner

ewels commented Mar 2, 2022

Yeah, I'm a bit in two minds as it's not all that much code to add I guess. It's more the fear of getting to the point where rich-click is rewriting the entire click library 😅

Do you have reasons other than the testing thing to continue using these instead of native rich functions?

@RhetTbull
Copy link
Author

Do you have reasons other than the testing thing to continue using these instead of native rich functions?

No, for me it's primarily driven by the fact that I want testing to work. The ideal solution is to figure out how to get rich fixed so it plays well with click and pytest. I'll poke around the rich code to see if I can figure out a fix there.

I came up with the "rich echo" while trying to solve the testing thing and thought it was kind of elegant as it allows existing click code to be slowly migrated to rich output without any big changes. It does to echo what rich did to print. (But as implemented it is a bit of hack).

@ewels
Copy link
Owner

ewels commented Mar 28, 2022

Hi @RhetTbull,

Apologies, I let this PR sit here a bit too long and it fell behind the main branch. I've just had a stab at resolving the merge conflicts and getting the code up to speed with the new linting tools. I've also moved the new code into its own file and rearranged the examples to match the new directory structure.

The only thing I haven't fixed yet is mypy, as the typing you added doesn't match up with what was added in another PR. Would you mind taking a look at this? As I'm not entirely sure what the best way to resolve it is.

Many thanks,

Phil

@RhetTbull
Copy link
Author

OK, I'll take a look.

@RhetTbull
Copy link
Author

I'm in the middle of another project but will take a look at this when I get a chance

@ewels
Copy link
Owner

ewels commented Apr 4, 2022

No rush 👍🏻

I think it should be fairly simple hopefully, I would do it myself but I'm not totally sure about how you're using these functions in the wild, which is why I was a little nervous. (Also I'm totally new to Python typing so there's a high chance that I would mess it up 😅 ). Let me know if you'd like me to have a stab at it anyway though.

@heeplr
Copy link

heeplr commented Mar 16, 2023

came here looking for this. A straight-forward rich capable click.echo() would be very handy.

@ewels
Copy link
Owner

ewels commented Apr 12, 2023

Hi @RhetTbull,

I just merged in #92 that @BrutalSimplicity says resolves this PR (#89 (comment)).

I can't see how it adds rich markup to these commands, but honestly the PR is massive and has gone a bit over my head.

If you get a moment, would you mind taking a look?

Thanks!

Phil

@dwreeves
Copy link
Collaborator

This is a neat feature and I'm looking to add it in rich-click 1.8, although I have a few notes.

  • I think we should make the kwargs explicit, as it works nicer with autocomplete in users' IDEs.
  • I think one of the kwargs should be the console.
    • And if one of the kwargs is the console, this may mean that the echo_via_pager does not need to be there, since the user will be able to do something like this:
      with console.pager():
          for msg in iterator:
              echo(msg, console=console)
      Alternatively, with the context passed, you can do something like this (although supporting this may require some minor refactors of the option highlighter):
      with ctx.make_formatter().console.pager():
          for msg in iterator:
              echo(msg, console=console)
    • Ideally I want to avoid adding something like echo_via_pager to keep the API surface cleaner. Since I think console is a reasonable kwarg and that adding it allows users to use the pager, this accomplishes the same goals with a smaller API surface, so I think it is preferable.
  • Regarding rich markup, I think the behavior should be something like this: there is a kwarg for the echo() function that is use_rich_markup: Optional[bool] = None. If kept as None, resolve via checking get_current_context / the global setting; if the boolean is set then use that instead.

I will get around to this in the next few weeks. Thanks for your work on this and sorry it took a whole year and a half!

@dwreeves
Copy link
Collaborator

dwreeves commented Apr 28, 2024

After thinking about this for a bit, I've come to the conclusion this may not be in scope for rich-click.

Apologies for reneging.

We appreciate your work on this PR, even though it was a long time ago. So I think given that you worked hard on this, we owe it to you to provide a brief explanation as to why we are not going to implement this feature:

1. Overhead

rich.console.Console() adds nonzero overhead to code execution. We'd have to lazily load it, create an API where the console is loaded lazily, perhaps from the context. There's a lot of setup to make this feature work.

2. console.print() is great and people should use it / learn it

I don't think we should encourage users to learn a separate API that is just a thin wrapper around Console().print(), when they could instead use Console().print() directly.

There is something to be said for having users learn a core API like Rich's Console() API. It's a project that has, as of writing, 47k stars on Github, and is so frequently used across the Python ecosystem that even pip has a vendored version of it.

Having familiarity with Console() is a handy little side-upgrade in your Python skill tree.

Meanwhile, rich-click is significantly less popular, and has a much less ambitious scope. I think it's a great tool (that's why I came on board as a maintainer), and people should use it, but my point is: it's less valuable for users to learn a niche feature of rich-click than it is to learn a core feature of Rich.

As software folks, I'm sure we can all appreciate that part of the value we get out of the time we spend doing things is not just the objectives we achieve, but learning the tools and APIs required to achieve those objectives. If you're going to spend time making arbitrary text pretty, Rich is the place to go.

3. Maintaining a wider API surface has a cost

The more things we add, the more things we need to maintain.

It's also a lot easier to add things than to remove them. Once we add something, it's unlikely we can ever really remove those features. Even really minor changes are frustrating.

For example, later today, I'm updating a Flask app I manage for a nonprofit. I wrote that Flask app 3 years ago. I'm getting deprecation warnings for some things and it's incredibly annoying. I just want to make a few updates and leave. These aren't even big lifts; it's just FLASK_ENV is deprecated and the thought of updating that really irks me.

I say this to point out the reality of all this:

  • adding things easy
  • removing things hard
  • users hate hate hate hate when you remove things and you force them to update their code (Flask asking me to update)
  • maintaining things for a really long time burns you out (me with my nonprofit's Flask app, but also, potentially one day, me with rich-click)

I do like maintaining this project and making it as good as it can be, but I'd also like to silo the amount of time this project takes up in my life. Adding a feature like this means that I have more stuff to maintain. And I don't want to take on that responsibility. Given how I'd like to use my limited time on this project, I'd rather rich-click be really good at a small number of things than be mediocre at a large number of things.

Not only am I protecting against the reality that one day I may burn out with this project, but by adding fewer features, I'm delaying the point where I burn out. Which is good! Less responsibility = more fun, and more fun = more regular, more prolonged maintenance.

This is a very human thing to discuss about a code project, but this code project is ultimately maintained by humans, not robots, so the human part matters.

4. Very little demand

I don't think there is much demand for this. The Console() made during help text rendering is somewhat special purpose and there are many good reasons to prefer using your own console for normal code execution. It's not clear to me (1) how many people are demanding to access a special, managed console via the context or some other construction. (2) How many people would then want that wrapped in the click.echo() API, rather than accessing normally.


Going forward, into 1.9, we can ask ourselves the question: is rich-click missing anything in terms of rich.console.Console integrations?

I think the main thing is: it's potentially a little confusing how the RichContext.console relates to the console that actually prints help text. This is partly because we are trying to fit a square peg into a round hole: Base Click's abstractions are built for writing directly to sys.stdout, not writing to a rich.Console, and the buffer that the help formatter writes to is for a more limited scope than.

Does this matter, really? For the most part, no. 99.9% of users and use cases do not require the Console that prints help text to share the Console that prints in your application.

The main place where this matters is rich-click --output ..., which outputs to html/svg. Right now, with 1.8 (which is releasing in a couple days), this is currently not asserted via unit tests or documented, and I'm not sure it works 100% of the time, but a user can do something really complex like this:

import os
from rich.console import Console
import rich_click as click

@click.command
@click.rich_config(console=Console(record=True))
@click.pass_context
def cli(ctx):
    if ctx.export_console_as is not None:
        ctx.console._file = open(os.devnull, "w")
    ctx.console.print("[red]hello, world![/]")

if __name__ == "__main__":
    cli()

And then you'll get this:

$ rich-click --output html hello:cli
hello, world!

$ rich-click --output html hello:cli
<span style="color: #800000; text-decoration-color: #800000">hello, world!</span>

You can try this yourself with the main branch.

Three things to me about this are obvious:

  1. This is an uncommon requirement; typical users can just DIY this by adding their own --output flags to a CLI and handling the logic themselves with a detached Console().
  2. It's not a zero in terms of its usefulness, and there's a strong argument rich-click should support doing this.
  3. The way to achieve this behavior is frankly absurd and "hacky." So if rich-click were to officially support this behavior, The API for doing so cannot look like this.

So that's kind of where my head is regarding what we should do about printing things. TLDR:

  • We're keeping click.echo() the same.
  • Users should be encouraged to use rich.console.Console() and most users don't need something special.
  • There is basically one case where accessing the ctx.console can be potentially useful and we'd rather focus some effort on making that case more robustly supported and having the API be more well thought-out, as opposed to doing something like rewriting echo() or adding a special echo_via_pager.

@dwreeves dwreeves closed this Apr 28, 2024
@heeplr
Copy link

heeplr commented Apr 28, 2024

Users should be encouraged to use rich.console.Console() and most users don't

Does rich.console play nice with all the click quirks? Maybe this issue would be a good place to refer to some example on how to use it with rich-click.

I wouldn't object your decision since you listed very good reasons and I fail to completely grasp all the consequences of that feature for the project, but I know those are often underestimated.

But I'd like to mention that I came to this issue since basically the first thing I missed was a way to print rich text "the click way" using markup. And the lack of a way to do it made me stop using rich-click (richness just wasn't important). So I wouldn't agree that there is no demand.

So I think if an example would basically look like a bad version of this PR, then (and only then) reconsideration might make sense since I see a rich echo() as vital feature of a rich framework. If Console().print() can be made work as a usable drop-in replacement for click.echo() that works without re-inventing this PR, then it totally makes sense not to add this as additional ballast. (Also this could be re-opened anytime so it's not really "wasted".)

@dwreeves anyways, thank you (and all contributors) for your time & effort!

@dwreeves
Copy link
Collaborator

dwreeves commented Apr 28, 2024

Does rich.console play nice with all the click quirks? Maybe this issue would be a good place to refer to some example on how to use it with rich-click.

I guess my question would be: Why not? Or maybe, what do you have in mind when you ask this?

  • During code execution, rich.console.Console() is just a separate object that prints to stdout. You can use it the way you use any other object.
  • During help text generation, Console() does not play very nicely with Click. And I should clarify what I mean by this, since I have something specific in mind: I mean this in the sense that it does not match 1-to-1 with the abstractions Click natively relies on to generate help text when a user invokes help text generation from --help. But that's where rich-click handles things. That's what rich-click is good at! And starting with the release of 1.7.0, continuing into 1.8 (releasing this week) and 1.9 (releasing end of year), we've been doing some work with the internals to align these two things better. But this is also very in-the-weeds stuff that impacts, generously, 1% of users.

You can make a Click CLI that is just print("hello world") or you can make the CLI an entrypoint to a web application, or a machine learning training routine, or a cron job for a data pipeline. You can even make a Click CLI that runs Doom.

Click does not do anything weird in terms of mutating how Python fundamentally operates. You can add and remove whatever you want. It just creates a thin and slightly interactive layer between your terminal and your code base. The same goes for rich-click.

It's possible we can mention this in the docs somewhere, maybe a paragraph or two saying something like: click.echo() is just the normal click.echo, we don't do anything special to it, please use console.print() instead if you want to use Rich, or use print() or logging etc. It's not clear exactly where this would fit into the docs right now, but I'm sure we'll find a spot as we flesh out API documentation, which right now is lacking.

But I'd like to mention that I came to this issue since basically the first thing I missed was a way to print rich text "the click way" using markup. And the lack of a way to do it made me stop using rich-click (richness just wasn't important). So I wouldn't agree that there is no demand.

Users have a few options: they can print rich text the Rich way, or they can print normal text the Click way, or they can print normal text the logging.Logger() or print() ways. (Or with the RichHandler, print rich logs that way.)

"print Rich text the Click way" during code execution is to me a bit of a strained idea. Don't take me saying this the wrong way, but there is nothing sacrosanct about click.echo(), in the sense that you should be able to cmd+shift+R your code base and replace every occurrence of click.echo( with a console.print(, plus add a console = Console() somewhere in the code, and that should be an acceptable workflow.

Anyway, I'm sorry this is disappointing. I appreciate your thanks to me and the other contributors!

@heeplr
Copy link

heeplr commented Apr 28, 2024

Good to know. Thank you for elaborating.

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