-
Notifications
You must be signed in to change notification settings - Fork 664
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
Add generic output formatter interface. #388
Conversation
I believe @meeuw's number-as-string is working now in this PR since tabulate has |
This is an excellent refactor. I love seeing the progression of changes as I step through the commits. I have a few questions that I'll sprinkle throughout the diffs. Nicely done! 👍 |
mycli/main.py
Outdated
for line in output: | ||
click.echo(line, nl=new_line) | ||
|
||
def format_output(self, title, cur, headers, status, expanded=False, | ||
max_width=None): | ||
table_format = 'expanded' if expanded else self.table_format |
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.
We used to store the table format on the Mycli
instance as self.table_format
. Now that we have OutputFormat
, we should place the table_format
on the instance of OutputFormat
. MyCli
class shouldn't be in the business of holding state for other objects.
When we want to override the table_format
based on the width, we'll simply overwite the value in the OutputFormat
instance.
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.
That's a good point. What do you think of having an optional keyword override for table format as well. For instance:
formatter = OutputFormatter(format_name='simple')
formatter.format_output(data, headers) # formats as simple
formatter.format_output(data, headers, format_name='expanded') # formats as expanded
mycli/output_formatter.py
Outdated
fkwargs.update(kwargs) | ||
preprocessor = fkwargs.pop('preprocessor', None) | ||
if preprocessor: | ||
data = preprocessor(data, **fkwargs) |
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.
The preprocessor
should get all the args passed into the format_output
. Right now we're only passing in the data
, but what if the preprocessor wanted to color the headers?
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.
Great point! We should definitely include headers in the preprocessor since they are part of the output.
041973f
to
5fb6f2a
Compare
e2aa7e0
to
3dbe5c0
Compare
Codecov Report
@@ Coverage Diff @@
## master #388 +/- ##
=========================================
Coverage ? 72.68%
=========================================
Files ? 29
Lines ? 2544
Branches ? 0
=========================================
Hits ? 1849
Misses ? 695
Partials ? 0
Continue to review full report at Codecov.
|
mycli/output_formatter.py
Outdated
self.register_output_format('expanded', expanded_table, | ||
preprocessor=override_missing_value, | ||
missing_value='<null>') | ||
|
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.
I think register_output_format should be called outside of OutputFormatter (from MyCli.__init__
?)
Or else we could also just directly initialize the self._output_formats dict like:
self._output_formats = {
'csv' : csv_wrapper, {preprocessor=override_missing_value, missing_value='null')
...
}
And remove register_output_format
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.
I think that's a good point.
I favor the idea of OutputFormatter
setting up the dict when it's initialized. My main reason for liking this is keeping all details of formatting away from MyCli (e.g. wrappers, format names, format packages, etc.).
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.
Hmm yes, that's true. Let's keep it this way. I was thinking of letting the user configure more output formatters at runtime but we can always refactor it by then (now it's too far fetched).
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.
I do think there is a lot of room for new features in the future. I think you're right that we should keep it simple for now.
09cd34f
to
152a16c
Compare
152a16c
to
434aea0
Compare
0ab3327
to
359044c
Compare
_output_formats = {} | ||
|
||
def __init__(self, format_name=None): | ||
"""Register the supported output formats.""" |
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.
This doc string is outdated. Looks like I missed it in my refactor.
Looks like we lost the ability to auto-complete on the |
@amjith Good catches. Looks like the background refresher wasn't setting the table formats when it created the |
I'm on linux (Ubuntu). When I choose 'Single' or 'fancy_grid' as the table format the output is completely garbled. Is anyone else seeing this? It works fine with other table formats. |
@amjith Yes, that happens to me as well on MacOS. I'll try to track down what's going on later today. |
@tsroten is has to do with the less pager and escape codes. If you use cat as a pager it works. |
8696bf7
to
cdbc7cd
Compare
48b1daa
to
8fa96a2
Compare
Are we ready to merge this or should we wait for cli_helpers to land? |
@amjith I'm up for it being merged now. Then, when CLI Helpers is released we can update mycli to use it instead. |
Description
OutputFormatter
class with all of tabulate's output formats.Checklist
changelog.md
.AUTHORS
file (or it's already there).