cmd2 2.5.0 changed the method chain for output

[kotfu]

To describe this issue, I need to define what I mean by "method chain for output". If you are a developer of a cmd2 based app, you would typically call one of the cmd2 methods to produce output, i.e, one of poutput(), perror(), pwarning(), pfeedback(), or ppaged(). The cmd2.py implementation of these methods appropriately called each other. For example, ppaged() makes a determination whether output is going to a terminal, if so, it opens a pipe and sends the output to your operating system pager (like less), but if output is not going to a terminal, ppaged() just calls poutput(). Similarly pwarning() applies a different style and then calls perror(). This is what I mean by the "method chain for output".

In cmd2 version 2.4.3 and prior, the method chain for output was:

• poutput() -> style_aware_write()
• perror() -> style_aware_write()
• pwarning() -> perror()
• pexcept() -> perror()
• pfeedback() -> poutput() or perror()
• ppaged() -> poutput() if not on a functional terminal

cmd2 version 2.5.0 made some material changes to this method chain. It introduces a new method print_to()

• print_to() -> ppaged() or style_aware_write()
• poutput() -> print_to()
• perror() -> print_to()
• pwarning() -> print_to()
• pexcept() -> perror()
• pfeedback() -> print_to()
• ppaged() -> style_aware_write()

What is the rationale for this new design? I have some specific questions:

• what's the purpose of the print_to() method and what guidance is there for developers for the use/overriding of that method?
• why does print_to() call ppaged()?
• why doesn't pexcept() call print_to()?
• why doesn't ppaged() call poutput() any more?
• why does pexcept() still call perror() but pwarning() doesn't?

Besides the philosophical questions to help me understand this better, this change broke my test suite because I had reimplemented poutput() to use rich.console. In v2.4.3 when you called ppaged() the call chain went to poutput() if there was no terminal present. In 2.5.0, ppaged() no longer calls poutput() so I couldn't intercept the output in the way I was previously doing. There is a relatively simple workaround for me in my app, I can just reimplement ppaged().

One other note, the method signatures for print_to() and style_aware_write() use a fundamentally different design than rich.console. Meaning, if I as a developer of a cmd2 based app wanted to use rich.console for my output, I need to not use any of the cmd2 output methods or I need to reimplement them all. Even more discouraging, if you are using rich.console, there is now no method you can reimplement and get all of the cmd2 generated output to go to rich.console

The cmd2 approach is based on file descriptors passed a parameters to the methods: you set the file descriptors to be where you want the output to go and then pass the file descriptor around to the various methods which write to that file descriptor. In rich.console you create a console object give it the file descriptor to use, but to generate output you must call methods on the console object.

[tleonhardt]

@kmvanbrunt I think you likely have the best insight into why these changes were made in 2.5.0. Can you please explain the rationale?

[kmvanbrunt]

These changes were made in #1269.

Questions

Since I wasn't the author of those changes, I'll answer @kotfu's questions as best as I can.

what's the purpose of the print_to() method and what guidance is there for developers for the use/overriding of that method?

I think the point of print_to() is to have one location where the printing is actually done and all the methods which wrap it only deal with text style. I believe the goal was to make it easier to swap out how things print (e.g. rich). Unfortunately, as you point out rich.console has a very different signature than other print functions so overriding print_to() is rather difficult without altering its signature.

I will say the print_to() method has been helpful on the 3.0.0 branch where I'm adding rich support. But I've had to rewrite it in a rich-specific way which still makes overriding it difficult. On that branch it doesn't matter though since cmd2 is built upon rich by default.

why does print_to() call ppaged()?

I assume it's because @anselor desired all printing functions to have the ability to page output while also wrapping print_to().

why doesn't ppaged() call poutput() any more?

I think because dest was added to ppaged() and it is not necessarily self.stdout.

why doesn't pexcept() call print_to()?
why does pexcept() still call perror() but pwarning() doesn't?

I don't know the answer to these.

Suggested Changes

I believe some of the changes in that PR need to be rolled back in the version 2 code because they fundamentally changed how the printing system worked to a level that constitutes a breaking change.

1. Remove style_output. See my comments about this in the PR.
   cmd2 will support a default text style via rich themes in 3.0.0. For now, a default text style is not needed in the version 2 code.

2. Remove pfailure(). This function is redundant since perror() does the same thing. I can see having a pfailure() function if we renamed style_error to style_failure and stopped applying text style within perror(). That can be revisited in the 3.0.0 branch, but would be a breaking change in version 2.

3. Remove paged and chop arguments from print_to(), poutput(), perror(), psuccess(), pwarning(), pfailure() and remove dest argument from ppaged(). Then ppaged() can call poutput() again. We can revisit the need for all print functions to page output in 3.0.0.

4. Only have poutput() and perror() call print_to(). The rest will wrap either poutput() or perror(). This restores the functionality developers are used to in the 2.0 version and won't break existing overrides.

[tleonhardt]

I think we should endeavor to stabilize the cmd 2.x branch at a point which is essentially backwards compatible with what developers have been using for years. Once we restore that, we can put future efforts into improving the 3.0 branch and make whatever backwards-incompatible changes there we think is best for the future.

I believe that means two main print functions - poutput and perror which all others wrap. I think this should restore the method chain and prevent breaking any more breaking changes for others.

I think it is fine to leave poutput and perror calling print_to instead of style_aware_write since that shouldn't impact anyone overriding poutput or perror. But I think we might need to modify how ppaged works to be similar to what it was before as well and hence modify print_to.

Basically I think I'm agreeing with what @kmvanbrunt suggests above for all changes 1, 2, 3 and 4.

@kotfu what do you think?

[kmvanbrunt]

I have a PR up which rolls back these changes in the 2.x branch.
Most of the changes will be preserved in the 3.0 branch though since they integrate quite well with rich.

[anselor]

Be careful how you did the rollback. If you did a git revert on it it'll also revert it when you merge the 3.x branch.

…

On Tue, Dec 17, 2024 at 11:27 AM Kevin Van Brunt ***@***.***> wrote: I have a PR up which rolls back these changes in the 2.x branch. Most of the changes will be preserved in the 3.0 branch though since they integrate quite well with rich. — Reply to this email directly, view it on GitHub <#1390 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEJC5SX4TTJ7HUNJ26CDHG32GBGGXAVCNFSM6AAAAABTQSFZJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBYHE2DOMZTGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[kmvanbrunt]

@anselor It was a manual change.