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

Regarding KISS and direction of the project #1251

Closed
GnikDroy opened this issue Oct 27, 2022 · 12 comments
Closed

Regarding KISS and direction of the project #1251

GnikDroy opened this issue Oct 27, 2022 · 12 comments

Comments

@GnikDroy
Copy link

Firstly, thank you for your excellent piece of software. cmd2 works well for my applications.

A case for KISS

I realize that it is rather unusual to request for the removal of a feature, but I think some of the features of cmd2 don't fit. A very clear example is the table creation API. As you already know, Textualize/rich is a project that also supports this, is popular, and is likely used in conjunction with cmd2. Please correct me if I'm wrong, but cmd2 isn't supposed to be a library to help render output of a command. But facilitate writing commands that can be used in a shell like environment.

Features like, autocomplete, history, argument processing, scripting, aliases fit well with the project. Features like table creation do not. I think table creation should be removed from cmd2 and delegated to a project like rich.

Similarly, the main Cmd2 class also has some features that might be better off separated into a plugin.
For example,

        include_py: bool = False,
        include_ipy: bool = False,

And all the relevant codepaths are better off in a separate plugin/mixin instead of the main Cmd2 class.

Maybe some stuff in ansi.py is also better off delegated to rich, or some other library? I am not sure.

A case for improvement.

A feature that is in scope for the project and potentially useful to the users is syntax highlighted help messages and argument options. Many people, including me, have been using this for quite some time, and I believe it should be the default (but customizable) behavior.

Example:

WindowsTerminal_iMgQcr0YEy

@tewhalen
Copy link

Agreed that I'd love to see the functionality of cmd2 (and similar tools!) split into separate libraries. I think there's a case to be made for figuring out how to disentangle (a) command discovery and argument completion, (b) a bunch of default/example commands, and (c) the REPL. For my project, I've looked at rich, click, typer, prompt_toolkit, among others. They all make assumptions at a fairly deep level about how they'll do input/output that make them very difficult to embed or reuse in other contexts. click and typer especially are amazing at command discovery and autocompletion but make foundational assumptions that they'll be parsing the command-line args of a program.

@tleonhardt
Copy link
Member

tleonhardt commented Jan 28, 2023

I am in favor of taking a dependency on rich and replacing most of cmd2's text formatting functionality with it.

Incorporating rich also opens up other doors to support things like better tracebacks, syntax highlighted source, progress bars, etc.

On the other hand, cmd2 has always tried very hard to minimize our dependencies so it can be used by as wide a user base as possible, including very unconventional situations where users don't have access to pip to install dependencies. So adding a new dependency, especially one with a lot of transitive dependencies, is not something we ever do lightly.

@kmvanbrunt @anselor @kotfu Do any of you have strong opinions one way or another?

FYI, this is somewhat a duplicate of #1216

@kotfu
Copy link
Member

kotfu commented Jan 28, 2023

I broadly agree with @GnikDroy's original sentiment. We've always wanted cmd2 to have "batteries included", but we also have to draw the line somewhere or we'll end up with emacs. The current state of cmd2 is:

  • we have some built-in support for syntax highlighting and text formatting
  • there is obviously demand from users of the library for something more and/or different
  • A developer can choose to use another library, but cmd2 will continue to use the built-in stuff for it's internally generated messages and this output can either not be intercepted, or it's difficult to intercept, or the formatting/styling will conflict with whatever the developer wants to do

My preference would be to not include any text formatting capabilities in cmd2. Instead of cmd2 depending on rich and providing a rich mixin, why don't we make it easy for developers to whatever text formatting/highlighing system they want.

If we were to go in that direction, it would mean something like:

  • remove ansi.py from cmd2
  • remove table_creator.py from cmd2
  • ensure cmd2 reliably and uniformly internally uses our existing poutput(), perror(), pfeedback(), pwarning(), pexcept() methods to send all output created by cmd2. As an example of where this isn't the case today, we have autocomplete code which outputs directly to sys.stdout. The implementation of these methods in cmd2 should not include any styling. Developers wishing to apply text formatting or styling to these various types of output can then create their own implementations of these methods (via their own mixin or directly in their subclass) which apply styling and formatting as they see fit, whether to use a simple library like ansicolors or a robust library like rich.
  • In addition to these core output methods, we may need to:
    • revise the way we use/generate the command prompt and secondary prompt
    • make sure cmd2 output streams are ansi clean, so if a developer marks up their output and asks cmd2 to push it to the output stream, we don't muck it up on the way
    • figure out how we can capture standard out from argparse and pass it through so a developer using cmd2 can apply their desired syntax highlighting to it
    • figure out if we can keep our "batteries included" tab completion code while still making it easy to apply syntax highlighting to the completions (this feels pretty tricky to me, we may not be able to solve this one)
    • I expect once we got into the work, we'd find more stuff to add to this list

If there is enough interest in exploring this, I'll be happy to create a branch and start working on it so we can at least see how it might work out.

@anselor
Copy link
Contributor

anselor commented Jan 29, 2023

As I recall, our table creator was added because it was needed for many of the tab completion formatting.

I was unable to comment earlier but much of what @kotfu suggests is similar to what I was thinking.
I'd rather not force a dependency on rich on everyone who uses cmd2. While rich is the currently popular library for this this could change again in the future. I'm not entirely sure a mixin is quite the right thing either.

Instead I'd propose that we have a defined output generation interface for the various forms of formatted output cmd2 internally depends on. We could have a progressive enhancement approach where adapters to libraries like rich would be used automatically if rich is present. The way the output adapter is chosen can be documented and overridden in situations where multiple libraries are installed or a custom one is desired for a particular situation. Our current p*() functions can feed to the output formatters. Our current built-in implementations can, then, be placed in a fallback output adapter implementation we provide as a pre-canned batteries-included solution.

@GnikDroy
Copy link
Author

GnikDroy commented Jan 29, 2023

Just to clarify a bit.

I am not advocating to replace the internal stuff with rich (worth serious consideration, though). Frankly, it is not the user's concern. What cmd2 uses internally to render tables doesn't matter. It is up to you guys to consider that.

What I'm advocating for is to remove them from the public API. Take table creation for instance. Cmd2's table creation API is exposed to the user (mentioned in the docs, methods are not private etc). So, that means every user has the option to choose between a dedicated library for these kinds of stuff like rich (which can also create tables) or cmd2. rich is just an example, there are other libraries that do this.

The user has to compare the pros/cons of rich and cmd2. This doesn't make sense though. Since they don't solve the same problem. One is used to build interactive CLI. The other is used to render output. So obviously, rich is better suited for rendering output.

I don't think exposing table creation is a great idea. Cmd2 doesn't promise to help you render output of commands. That is the responsibility of libraries like rich.

Additional rendering stuff that is exposed to the user:

Is it really necessary to expose this to users?

Personally, I think helping users render output of commands is not cmd2's responsibility. Rich is a library tailored to do exactly this.

@tewhalen
Copy link

I think the key difficulty here is that it rapidly starts to become impossible to disentangle the core of cmd2 from output rendering - consider the example up at the top! Presumably we all still want "core cmd2" to automatically produce command help, as well as do things like command- and argument-completion, and produce/update a prompt – but all of that implies deep dependencies on the output rendering. At some point, we'd find ourselves re-coding all of cmd2's output into a markup format that can then be processed by an output library and rendered appropriately (and then needing to write some kind of standard lowest-common-denominator output rendering for cmd2 to fall back on). We might then wonder whether it wouldn't just be easier to depend on a single library that does the terminal-appropriate rendering already, given markup input. Completion and prompt also have that problem, although possibly worse, since it's actually interactive.

@anselor
Copy link
Contributor

anselor commented Jan 30, 2023

As @kotfu noted above, this project has strived to be a 'batteries-included' solution to building a command shell. As such, some form of output rendering will always be provided. Any of the formatting functionality we've implemented we've opted to expose so smaller projects can use it if they choose to. I know of projects that use the built-in output formatting because it's easy and good enough for basic needs. I do think that making it easier to override the output formatting to leverage each project's text rendering library of choice is a worthy goal moving forward.

That being said, being mindful of feature/scope creep is also a good idea. The last thing we want is for us to become a bloated mess of a project from ever-increasing scope.

I'll note that the py and ipy commands have been a part of cmd2 for many years - predating many of the newer enhancements that make it easier to modularize commands. It probably makes sense for us to transition many of the built-in commands into CommandSets that are pre-loaded in the default case for now and eventually transition them to be only loaded if explicitly requested.

@kotfu
Copy link
Member

kotfu commented Feb 7, 2023

@GnikDroy, is the source code for your highlighted help message available online? I'd like to see how you did it and see if I could integrate something similar into cmd2.

@GnikDroy
Copy link
Author

GnikDroy commented Feb 7, 2023

@kotfu https://gist.github.com/GnikDroy/e6d74bef43871935df08cf6de979e680

COLOR_SCHEME = {
    "info": "yellow",
    "warn": "red",
    "success": "green",
    "list": "cyan",
    "banner": "red"
}

randomize_arg_color=True

WindowsTerminal_3ar3b6MR3A

randomize_arg_color = False

See original image.

@kotfu
Copy link
Member

kotfu commented Feb 15, 2023

I've done several experiments to see how to use rich to highlight help messages.

I tried using regular expressions to highlight the output: couldn't reliably highlight all the parts of the help I wanted.

I tried feeding highlighted arguments to argparse.ArgumentParser: works for prog and argument help, but not for anything else.

I tried subclassing argparse.ArgumentParser like @GnikDroy: there are so many places where it does width calculations that you'd have to reimplement most of it in order to get equivalent output.

Then I found what I think is the best answer, and best of all, someone already did the work: the rich_argparse module provides a rich powered implementation of argparse.HelpFormatter. In my limited testing, it's by far the best solution. I haven't yet done any work to see what it will take to make this work with the custom argparse stuff in cmd2, but that's what I'll work on next.

@anselor
Copy link
Contributor

anselor commented Feb 15, 2023

@kotfu I made a number of customizations to how arguments are grouped to make them clearer. I imagine what would make sense is to make the same customizations of the rich powered HelpFormatter and then swap out which we use by default depending on whether rich is present.

@GnikDroy
Copy link
Author

GnikDroy commented Feb 15, 2023

Worth noting that argparse.HelpFormatter is private. Only the name is public. This is also the reason I refrained from subclassing HelpFormatter.

It might still be worth it to do this though.

@GnikDroy GnikDroy closed this as completed Jun 8, 2023
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

No branches or pull requests

5 participants