Basic table support #55
Conversation
srepmub
force-pushed
the
floeper
branch
4 times, most recently
from
19752aa to
0dc643e
Compare
|
Hi, Thanks for your contribution! For a PR to be accepted, it needs to provide a set of tests to exercise the new code. It would help a lot in explaining the expected data, facilitate experimenting the proposed patch and prevent regressions in future updates. |
|
hi francois, thanks for your quick response. I will see if I can add some basic tests. |
srepmub
force-pushed
the
floeper
branch
2 times, most recently
from
1272ca8 to
4e9fb3e
Compare
|
okay, added some tests. |
|
I also added support for internal ilnks/bookmarks. |
|
fixed indentation for multi-line code tag (and improved the test) |
There was a problem hiding this comment.
Thanks for the suggested improvements! I think they all make sense, but would prefer to keep the review focused by splitting separate improvements in separate PRs (i.e. one PR for skip style data and one PR for add line breaks for <code>).
I only reviewed the “Basic table support” in this PR. Could you please open PRs for other improvements?
Additionally, isort changes should have been fixed with 345cf86 and can be dropped.
This comment has been minimized.
This comment has been minimized.
|
sorry for the late response, I was on vacation. I will look at your comments and submit separate PR's, but it may take some time.. :-) |
|
please have a look at this version. it adds several things as you requested, and it should also work more or less for nested tables. if this can be merged (after further changes perhaps), I would be interested in looking into taking layout and color styling from the HTML side. also happy to cleanup the other smaller patches that I sent in earlier. |
srepmub
changed the title
There was a problem hiding this comment.
Hey @srepmub,
Sorry for the delay. This patch is working well in my testing.
It’s sad that the table autofit option does not resize the columns to their optimal width.
Column widths are adjusted in either case if total column width exceeds page width.
We could abuse the documented behavior to automatically size columns by adding huge columns, but your solution is more straightforward.
I think this patch should be good to go once tests are ported to the existing test_html2docx.
| col += 1 | ||
| self.tables[-1] = (table, row, col) | ||
| if col >= len(table.columns): | ||
| table.add_column(0) |
There was a problem hiding this comment.
A comment explaining that columns must have a width to form a valid docx would be very helpful. It could also explain that the column width is computed in finish_table, because the total column count is needed to evenly spread the width.
|
|
||
|
|
||
| def test_table(): | ||
| html_path = os.path.join(TEST_DIR, "table.html") |
There was a problem hiding this comment.
Can test_html2docx be extended to accept tables in the spec?
The specs would need to be updated to specify paragraphs and tables in a document.
Since table cells can contain other tables or multiple paragraphs, we could recursively validate paragraphs and tables in a container (for now, a document or table cell).
| @@ -78,6 +78,8 @@ def _reset(self) -> None: | |||
|
|
|||
| # Formatting options | |||
| self.pre = False | |||
| self.table_cell: Optional[Any] = None | |||
There was a problem hiding this comment.
| self.table_cell: Optional[Any] = None | |
| self.table_cell: Optional[_Cell] = None |
| @@ -78,6 +78,8 @@ def _reset(self) -> None: | |||
|
|
|||
| # Formatting options | |||
| self.pre = False | |||
| self.table_cell: Optional[Any] = None | |||
| self.tables: List[Tuple[Any, int, int]] = [] | |||
There was a problem hiding this comment.
| self.tables: List[Tuple[Any, int, int]] = [] | |
| self.tables: List[Table[Any, int, int]] = [] |
| if self.table_cell is not None: | ||
| table = self.table_cell.add_table(rows=0, cols=0) | ||
| else: | ||
| table = self.doc.add_table(rows=0, cols=0) |
There was a problem hiding this comment.
| if self.table_cell is not None: | |
| table = self.table_cell.add_table(rows=0, cols=0) | |
| else: | |
| table = self.doc.add_table(rows=0, cols=0) | |
| container = self.doc if self.table_cell is None else self.table_cell | |
| table = container.add_table(rows=0, cols=0) |
| @@ -78,6 +78,8 @@ def _reset(self) -> None: | |||
|
|
|||
| # Formatting options | |||
| self.pre = False | |||
| self.table_cell: Optional[Any] = None | |||
| self.tables: List[Tuple[Any, int, int]] = [] | |||
There was a problem hiding this comment.
Since the table is only appended to, we could only keep track of the column and always use the last row. NBD either way.
|
Any chance this can be merged soon? I would super appreciate this feature! |
|
It seems this PR has stalled. Feel free to fork it and carry on! |
|
Closing because there has been no activity on this PR in over a year. |
hiya,
after these changes, an HTML document that I'm testing with comes out pretty nicely.
cheers,
mark.