Refactored bottom toolbar and rprompt functionality.#1700
Conversation
work the same way as get_rprompt(). Removed default implementation of get_bottom_toolbar() from Cmd class and moved it to the getting_started example.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1700 +/- ##
==========================================
+ Coverage 99.58% 99.60% +0.01%
==========================================
Files 23 23
Lines 5773 5758 -15
==========================================
- Hits 5749 5735 -14
+ Misses 24 23 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Other than the two very minor issues I called out, I don't have problems with the code changes per se.
However, I have a big problem with the new default behavior from a UX perspective. Before this change, when a bottom toolbar wasn't requested, there was no visual display of a bottom bar. Now, it displays an empty gray bar which is ugly, distracting, and pointless.
Please fix it so the default behavior is like it was previously.
Added enable_rprompt flag.
kmvanbrunt
changed the title
| - **disabled_commands**: commands that have been disabled from use. This is to support commands that are only available during specific states of the application. This dictionary's keys are the command names and its values are DisabledCommand objects. | ||
| - **echo**: if `True`, each command the user issues will be repeated to the screen before it is executed. This is particularly useful when running scripts. This behavior does not occur when running a command at the prompt. (Default: `False`) | ||
| - **editor**: text editor program to use with _edit_ command (e.g. `vim`) | ||
| - **enable_bottom_toolbar**: if `True`, enables a bottom toolbar while at the main prompt. (Default: `False`) |
There was a problem hiding this comment.
The newly added enable_bottom_toolbar and enable_rprompt (as well as refresh_interval below) are documented here as instance attributes, meaning they are instance members of the cmd2.Cmd instance that developers can override at runtime. However, in cmd2.py, they are strictly initializer arguments passed directly to PromptSession and are not stored on the Cmd instance. Attempting to override them at runtime will have no effect.
I think we should create a new section documenting __init__ parameters which are not also instance attributes and move them there.
This also applies to complete_in_thread.
Alternatively, we could restore them to instance attributes stored in Cmd.
|
|
||
| # Create the main PromptSession | ||
| self.bottom_toolbar = bottom_toolbar | ||
| self.complete_in_thread = complete_in_thread |
There was a problem hiding this comment.
Since complete_in_thread, refresh_interval, and bottom_toolbar were previously public instance attributes on Cmd (and documented as such), removing them from the object instance will break downstream code that attempts to access or modify them. This should be explicitly noted in the breaking changes section of CHANGELOG.md, e.g.:
- Breaking Changes
- ...
- Removed `bottom_toolbar`, `complete_in_thread`, and `refresh_interval` as instance attributes of `Cmd`. They are now strictly `__init__` parameters.Alternatively, we could restore them to being instance attributes.
| # Test overridden | ||
| expected_text = "bottom toolbar text" | ||
| base_app.get_bottom_toolbar = lambda: expected_text | ||
| assert base_app.get_bottom_toolbar() == expected_text |
There was a problem hiding this comment.
Overriding base_app.get_bottom_toolbar with a lambda and then asserting it returns the lambda's value only tests Python's ability to assign a function. Since the method no longer contains default logic, testing that the default implementation returns None is sufficient.
|
|
||
| expected_text = "rprompt text" | ||
| base_app.get_rprompt = lambda: expected_text | ||
| assert base_app.get_rprompt() == expected_text |
There was a problem hiding this comment.
Similar to test_get_bottom_toolbar, overriding get_rprompt with a lambda and asserting its return value tests Python's assignment capabilities rather than actual cmd2 functionality.
2 participants
Also closes #1699