-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Markdown table support and corresponding test #2401
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @thibaudcolas, @sarahboyce — I’ve added support for Markdown tables in the Entry model and included a test case ( |
sarahboyce
left a comment
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.
adamzap
left a comment
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 is very close!
blog/models.py
Outdated
| # baselevel matches `initial_header_level` from BLOG_DOCUTILS_SETTINGS | ||
| TocExtension(baselevel=3, slugify=_md_slugify), | ||
| "markdown.extensions.tables", | ||
| TableExtension(), |
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.
These two lines are doing the same thing. The first section of the documentation on extensions shows multiple ways to specify an extension:
markdown.markdown(some_text, extensions=[MyExtClass(), 'myext', 'path.to.my.ext:MyExtClass'])
So we can just pass in "tables" here since we're not customizing the extension.
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.
Thanks @adamzap for the clarification! Removed the duplicate tables extension and now using "tables" shorthand as no customization is needed.
d25bd4b to
e4f5c44
Compare
blog/models.py
Outdated
| from django_hosts.resolvers import get_host, reverse, reverse_host | ||
| from docutils.core import publish_parts | ||
| from markdown import markdown | ||
| from markdown.extensions.tables import TableExtension |
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.
@ahmedasar00 CI is failing because you no longer need this import.
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.
Thanks @adamzap for pointing that out! I've removed the TableExtension import and usage because it's no longer needed—using the "tables" shorthand works fine and avoids import issues. CI should pass now.
449c205 to
acfc6c9
Compare
adamzap
left a comment
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.
👍 Looks great!
|
Hi @adamzap , @sarahboyce Thank you so much for the helpful review and guidance on this PR — I really appreciate the clarity of your feedback. As someone who’s still learning how to contribute effectively to Django, I’d love to ask (if you don’t mind): do you have any advice or recommendations on how I can improve my future contributions, whether in code quality, tests, or choosing good issues to work on? Your experience and work on the project are truly inspiring, and any guidance would mean a lot to me. Thanks again, and really appreciate your time! |
|
This was a good issue choice 👍
|
|
@ahmedasar00 Thank you for the kind words and also for your contribution! In addition to what Sarah said:
|
|
Thank you very much, @sarahboyce and @adamzap! I really appreciate your time, support, and valuable feedback. 🙏 |

test_markdown_table_conversion) to ensure proper HTML conversion of Markdown tables.Fixes #2314