-
Notifications
You must be signed in to change notification settings - Fork 75
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
Printing standardized output for CLI commands #3323
Conversation
PR ChecklistHow to use this checklistHow to use this checklistPR AuthorFor each section, check a box when it is true. PR ReviewerCheck that the PR checklist action did not fail. Bug ReferencesNone. Confirm
How to properly reference fixed bugs
Test UpdatesUnit Tests
Integration Tests
Documentation
Does this PR require review from someone outside the core ubuntu-pro-client team?
|
@@ -133,11 +133,12 @@ def _get_column_sizes(self) -> List[int]: | |||
return column_sizes | |||
|
|||
def __str__(self) -> str: | |||
rows = self.rows |
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.
why do we need the rows
variable if the fix is applied on new_rows.append
call ?
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.
Because the new_rows thing is only done if the table exceeds the line length. If there is no line exceeding the available space, then the whole thing short-circuits to using the actual content in self.rows, without change.
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.
As part of our process to get ready for v35, I have rebased next-v35
onto main
. As a result, next-v35
is now obsolete. Please rebase this on main
and target the PR to main
.
dca3230
to
f921d17
Compare
Hopefully rebased this correctly against |
f921d17
to
c53ac05
Compare
@lucasmoura I have created the ABC to represent Blocks and Tables and something else. That suffices for not caring about the isinstance call if we want to add new representations or derivatives of those in the future - so I think this is enough for the call; no need to convert anything nonstandard to an instance of that class. PTALALMK |
bad, bad |
Make the __str__ method work even when max_length is changed after the instance is created Signed-off-by: Renan Rodrigo <[email protected]>
Signed-off-by: Renan Rodrigo <[email protected]>
Instead of passing it to the constructor, optionally pass it to a to_string method Signed-off-by: Renan Rodrigo <[email protected]>
Indented Blocks standardize indentation through the CLI, and Suggestion Blocks take the suggestion configuration into consideration. Signed-off-by: Renan Rodrigo <[email protected]>
Add a helper function to remove unwanted symbols in the CLI outputs. Signed-off-by: Renan Rodrigo <[email protected]>
This eases the isinstance check for Blocks, Tables and whatnot by creating an abstract superclass which represents those, defining to_string in its interface for consistency. Signed-off-by: Renan Rodrigo <[email protected]>
b3870e7
to
de872ee
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.
Thanks!
Why is this needed?
This PR solves all of our problems because it brings some tweaks to the Table implementation, includes the indented Block and SuggestionBlock implementation and adds a helper function which considers the formatter config when returning text.
This is the last base PR for the CLI before the commands start to appear.
Test Steps
Unit tests cover the functionality
Integration and such will come with the commands