Skip to content

Formatter improvements#16

Merged
tleonhardt merged 3 commits intomasterfrom
mix_formatters
Jun 28, 2018
Merged

Formatter improvements#16
tleonhardt merged 3 commits intomasterfrom
mix_formatters

Conversation

@anselor
Copy link
Copy Markdown
Contributor

@anselor anselor commented Jun 28, 2018

row_decorator function can now be provided to tableformatter to colorize row text
column obj_formatter and formatter can now be chained together to do column data processing prior to column display formatting.

anselor added 2 commits June 26, 2018 17:51
…be chained together.

This allows an value generated by an object formatter to be passed to an existing display formatter function.
For example, this allows a computed float value to from an object formatter to go through a display formatter to round/truncate digits.
… to return the per-row options. Currently the only supported option is text color
@anselor anselor requested a review from tleonhardt June 28, 2018 15:55
Copy link
Copy Markdown
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very helpful API change and the code looks fine.

I definitely recommend changing the usage of the term "decorator" to something like "stylist" since it isn't consistent with what is meant by a decorator in Python (though it is fully consistent with the GoF design pattern).

Comment thread examples/formatters.py Outdated
print("num out of range")


def decorate_row_obj(row_obj: MyRowObject) -> dict:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I grok that you are applying the object-oriented decorator pattern here, however in Python the term decorator has a very specific meaning.

In Python, a decorator is a function which takes another function as an argument and extends the behavior of that latter function without explicitly modifying it because it returns the modified function:

def a_decorator(func: Callable) -> Callable:
    """Do something here to modify func and return the modified version"""

Given that your "decorator" functions are not Python decorators, I would very strongly suggest calling them something else to avoid confusing end users.

This comment applies throughout this PR.

Hmm... how about "stylist" or "style" (depending on grammatical usage) as a synonym which accurately reflects their intent?

Comment thread tableformatter.py Outdated
columns: Collection[Union[str, Tuple[str, dict]]]=None,
grid_style: Optional[Grid]=None,
transpose: bool=False,
row_decorator: Callable=None) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we could have row_stylist, for example ...

Comment thread tableformatter.py


def Row(*args, text_color: TableColors=None):
def Row(*args, text_color: Union[TableColors, str]=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to elaborate in the docstring what kind of str might get passed in for text_color - i.e. it can take ANSI escape codes, but isn't looking for something like 'green'.

@tleonhardt tleonhardt added the enhancement New feature or request label Jun 28, 2018
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 28, 2018

Codecov Report

Merging #16 into master will decrease coverage by 0.22%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
- Coverage   88.27%   88.05%   -0.23%     
==========================================
  Files           1        1              
  Lines         819      829      +10     
==========================================
+ Hits          723      730       +7     
- Misses         96       99       +3
Impacted Files Coverage Δ
tableformatter.py 88.05% <88.46%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b45af1...e4da289. Read the comment docs.

Comment thread tableformatter.py
columns: Collection[Union[str, Tuple[str, dict]]]=None,
grid_style: Optional[Grid]=None,
transpose: bool=False,
row_tagger: Callable=None) -> str:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new name works for me. Thanks for the change.

@tleonhardt tleonhardt merged commit ab93419 into master Jun 28, 2018
@tleonhardt tleonhardt deleted the mix_formatters branch June 28, 2018 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants