Skip to content

Support for custom Pygments formatter #1187

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

Merged
merged 8 commits into from
May 9, 2022
Merged

Conversation

sharat87
Copy link
Contributor

Hi team!

This adds configuration support for using a custom Pygments formatter, either by giving the string name, or a custom formatter class (or callable).

Inspired by #490, which wasn't merged back then because v3 work was going on. I'm hoping we can merge this feature now please?

Thank you for your time.

This adds configuration support for using a custom Pygments formatter,
either by giving the string name, or a custom formatter class (or
callable).
@waylan
Copy link
Member

waylan commented Oct 27, 2021

This looks like a great start. Before we merge this, at a minimum, we will need to address the failing tests.

  1. As this is a new feature, it will require us to move up to a new MINOR version (version 3.4; see the Contributing Guide). Therefore, a new file will need to be created at docs/change_log/release-3.4.md for the release notes, and those release notes need to be added.
  2. The word formatters should be added to .spell-dict so that it is recognized as a correctly spelled word.
  3. Some tests should be added to tests/test_syntax/extensions/test_fenced_code.py which confirm that the new API works as advertised.

Finally, I wonder if perhaps we should have some error handling to deal with bad string names and/or objects (classes/callables) which don't properly match Pygments' API. As a reminder, a user passing text to Markdown should never result in an error. However, creating an instance of the Markdown class certainly can. So any error checking usually needs to happen when the extension is loaded, not when it is passed user input. If that is not possible or practical (as I suspect is the case here), then perhaps we should fall back to a default formatter on error.

@sharat87
Copy link
Contributor Author

Understood. Thanks for detailed pointers @waylan. I checked the contributing guidelines that GitHub pointed me to, but the one you linked to has much more information. I'll get on this right away.

@sharat87
Copy link
Contributor Author

Tests now pass with Pygments installed and with Pygments uninstalled.

Screenshot 2021-10-28 at 11 45 12

@sharat87
Copy link
Contributor Author

Hey @waylan, I think I've covered all the feedback from your message. Please let me know if I've missed anything or if there's more changes you'd like before merging in.

Also, could you please run the workflows again so I can see if we get all green now? Thanks!

@sharat87
Copy link
Contributor Author

sharat87 commented Nov 2, 2021

Fixed lint and spelling failures in the previous commit and pushed.

@waylan waylan added approved The pull request is ready to be merged. extension Related to one or more of the included extensions. feature Feature request. labels Nov 3, 2021
@waylan waylan added this to the Version 3.4 milestone Nov 3, 2021
@waylan
Copy link
Member

waylan commented Nov 3, 2021

This looks like it is ready to go. Thanks for doing the work. However, we have a bunch of bugfixes waiting for a bugfix release. Once we do that release, we can merge this. I've labeled this and added it to the correct milestone so that we don't forget about it.

@sharat87
Copy link
Contributor Author

sharat87 commented Nov 3, 2021

Thank you for sharing the merging plan. I'll look forward to the 3.4 release then.

@waylan waylan mentioned this pull request Feb 3, 2022
@waylan
Copy link
Member

waylan commented May 5, 2022

I will merge this if the conflicts are resolved.

@waylan waylan added the requires-changes Awaiting updates after a review. label May 5, 2022
@sharat87
Copy link
Contributor Author

sharat87 commented May 7, 2022

@waylan, thanks for the notification. I've just resolved merge conflicts.

@waylan waylan removed the requires-changes Awaiting updates after a review. label May 9, 2022
@waylan waylan merged commit 9d90d47 into Python-Markdown:master May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved The pull request is ready to be merged. extension Related to one or more of the included extensions. feature Feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants