-
Notifications
You must be signed in to change notification settings - Fork 38
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
fix/exception-in-console #215
Conversation
dae982c
to
af7b401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, thank you for your PR. Your description of the issue makes sense, although I have one note on the test changes.
af7b401
to
0fa7a62
Compare
Thanks for the change! It seems correct to me looking at the code, but I'll need to manually test before merging. A simple reproducible example will help with that, otherwise I'll make my own example and review the change on my own time (hopefully end of this week?). Don't worry about adding a unit test if you don't want to because we need to eventually revamp the entire testing approach, so until then we are OK with no coverage. Just an example works. |
0fa7a62
to
c7e8b8e
Compare
c7e8b8e
to
6618bfb
Compare
Hi, sorry for the delayed response—got caught up with some personal work! I've also noticed a few other drawbacks and have pushed an update. I believe that whenever we're using rich_console in context settings, everything should consistently fall under the rich_console. It didn’t seem logical to only use the console in these scenarios when a rich context is present, so I’ve made this adjustment accordingly. Here’s an example to demonstrate the change:
When the line context_settings = None is uncommented, the help error will behave as usual. Regarding the test case, I’ll address that in a future update when we revamp the testing approach, as you mentioned. For now, I hope the example provided helps with the review. Let me know your thoughts! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Very cool. Ran it and it works great! Thank you for your work on this @sankarngrjn, I will get it out shortly. |
Previously, when passing rich_console in context_settings, all outputs except exceptions were rendered under the provided console. This caused inconsistencies in the output format, as exceptions would bypass the rich console, leading to a mismatch in styling or behavior.
This change ensures that exceptions are also captured and displayed within the provided rich_console. By addressing this, all outputs, including exceptions, will now conform to the same console formatting, maintaining consistency across the board.
Ref. Past
Ref. Present