Add preliminary support for tables #290
Conversation
|
Haven't taken a deeper look yet, but could you please not abbreviate table to tbl? It's easier to read written out. |
|
Done. I was doing so to avoid collision with pulldown-cmark tag
enums, but indeed it looks much better now without the
abbreviation.
|
There was a problem hiding this comment.
Thanks a lot for this patch 🙏❤️
I'd not like to hold this off with a lengthy review, since any support is better than no support 😅
I've just had a few minor remarks on the doff, and some overall points:
- I'd appreciate a small test, which renders a small file with two tables to assert the formatting and to check that we properly reset the current table state at the end of a table.
- Please do add a changelog entry.
- Please check the manpage file and the README and the top level api docs whether we need to update them now that we support tables (I believe this restriction is mentioned in a couple of places).
Tests (edit)
Tests work mostly with golden files, i.e. the tests generate files which I check manually and then add to the repo; future test runs then compare against the existing golden file.
For this PR you'd add a e.g. tables.md to pulldown-cmark-mdcat/tests/render/md/samples/, then run the tests with $MDCAT_UPDATE_GOLDEN_FILES set to generate the initial golden file for tables.md.
Note that no other file should probably change, as tables aren't yet tested anywhere. If any other golden file changes you'd need to investigate.
|
Thanks a lot for the review! Honestly I did not expect that this PR would be merged – I was planning to rewrite it after your |
|
If you wait for blockwise rendering you'll wait a long time. I don't have much time for this kind of stuff these days, so if I get this done this year, it's already fast 😅🙈 Let's merge what we have ✌️ |
pengjiz
force-pushed
the
feat/table
branch
2 times, most recently
from
89da46c
to
57c0030
Compare
With this commit, mdcat can render basic tables. However, the support is incomplete. E.g., inline markups are stripped, overlong texts are not wrapped, and block attributes are mostly ignored. We intend to support those in the future. So we chose to not use a TTY table rendering library because it would be hard to plug the library into our existing renderer.
|
Sorry for the delay. I got a bit busy in the past few weeks. I just updated the implementation and then squashed and force pushed to my branch. It is ready from my side, and I would appreciate a review. Thank you! PS here is an updated screenshot rendering the README.md file:
|
|
Thanks for finishing this. Now I'm a bit busy, but I hope to have a look by the end of this week. If you haven't heard from me beginning of next week, please ping me 😊 |
|
@pengjiz clippy complained 😬 Could take a look? Once the pipeline is green, I'll merge and publish a new release |
|
clippy complained 😬 Could take a look?
Done. I think it is a new rule of clippy.
Once the pipeline is green, I'll merge and publish a new
release.
Thank you for reviewing and approving this PR, and also for
maintaining this awesome tool!
|
(Re: #2, #289)
Hello!
I added a very basic implementation for table rendering.
The implementation does not depend on any existing TTY table rendering libraries because I realized that we need to support, e.g., markups inside table cells and attributes, but I did not find a library that supports such a use case well.
Right now it is only a quick and dirty implementation. Many functionalities are missing, e.g., cell alignment, head row formatting, table rules, and markups inside cells. However, I noticed that you had created a
blockwise-renderingbranch, which might be very helpful for table rendering as well. So I am opening this PR to get some comments and hopefully provide some insights on implementing the new blockwise rendering.Thanks!
PS here is the table in README rendered with this implementation:
Nothing fancy indeed.