Support Markdown tables without padding#71
Conversation
|
@thombashi could you take a look at this one? 🙇 |
|
@thombashi could you take a look at this PR? 🙇 |
thombashi
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing.
- Should
MarkdownTableWriterdefault to no padding now, in a future (breaking) release, or never?
I don't plan to change the default value.
This is because padding generally improves readability, and changing it would break backward compatibility.
- Would it make sense to have a different argument for this behavior? Reference documentation doesn't appear to show parent class properties (
is_padding) but it seems like it should? 🤔
Since is_padding is declared as .. py:attribute::, it is intended that it does not appear in the subclass documentation.
Generally, users do not need to modify is_padding in individual writer classes.
Since this PR's changes affect only the MarkdownTableWriter class, it might be better to add a docstring to the Args of the MarkdownTableWriter.write_table method.
- Would an example in the docs be valuable (e.g.,
md_example_without_padding.txt)?
It would be helpful for users to include an example of using is_padding=False with MarkdownTableWriter.
| padding_len = self._get_padding_len(col_dp) | ||
| # columns in a header separator row must have at least one dash | ||
| # using a minimum of three accounts for right and center alignment | ||
| padding_len = max(3, self._get_padding_len(col_dp)) |
There was a problem hiding this comment.
I think it would be best to declare 3 as a constant, such as _MIN_HEADER_SEP_LEN .
| # columns in a header separator row must have at least one dash | ||
| # using a minimum of three accounts for right and center alignment |
There was a problem hiding this comment.
What I'd like to emphasize here is that, whether padding is enabled or disabled, a Markdown separator header must consist of at least 3 characters.
| # columns in a header separator row must have at least one dash | |
| # using a minimum of three accounts for right and center alignment | |
| # Columns in a header separator row must contain three or more characters; | |
| # valid examples include (---, --:, :-:). |
This PR makes it possible to render markdown tables without padding using the existing
is_paddingargument.Rendering a table with a header or value that is extremely long results in an excessively padded table. While the resulting table is (potentially) easier for humans to read, it is functionally equivalent1 once the markdown is rendered.
When using the table somewhere with limited space (e.g., a GitHub issue/PR body) not having the padding allows for a "larger" table (no characters wasted on padding) or more content in addition to the table.
Note
The
is_paddingargument removes the padding today but the header separator row requires at least one-for proper rendering. With this change you can write a valid markdown table, without padding.👀 Usage and Examples
Pass
is_padding=FalsetoMarkdownTableWriterto render a table without padding. The existing default behavior (table with padding) remains unchanged.These two tables are equivalent when rendered:
With padding (current behavior)
Rendered
Without padding (new behavior, with
is_padding=False)Rendered
💭 Other thoughts
MarkdownTableWriterdefault to no padding now, in a future (breaking) release, or never?is_padding) but it seems like it should? 🤔md_example_without_padding.txt)?Footnotes
At least for GitHub Flavored Markdown, and I wasn't able to find anything stating that CommonMark or kramdown require padding for tables. ↩