Skip to content
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

Facilitate extending HTMLRenderer to add attributes #134

Closed
wants to merge 2 commits into from

Conversation

not-my-profile
Copy link
Contributor

This PR introduces optional attr parameters to some methods of the HTMLRenderer, allowing API users to easily add their own attributes without having to reimplement the logic of the renderer methods. (An example for that is provided in the docstring and I also added two test cases).

Copy link
Collaborator

@pbodnar pbodnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice piece of code, just a few details to be discussed, thank you.

mistletoe/html_renderer.py Outdated Show resolved Hide resolved
mistletoe/html_renderer.py Outdated Show resolved Hide resolved
mistletoe/html_renderer.py Outdated Show resolved Hide resolved
mistletoe/html_renderer.py Outdated Show resolved Hide resolved
mistletoe/html_renderer.py Outdated Show resolved Hide resolved
mistletoe/html_renderer.py Outdated Show resolved Hide resolved
mistletoe/html_renderer.py Outdated Show resolved Hide resolved
@not-my-profile
Copy link
Contributor Author

I have an idea how to make this new API even nicer, but before I do, we should resolve #135.

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Feb 14, 2022

I updated the PR. Whereas previously attributes were either a dictionary or a string or None, now they are just dictionary or an empty tuple. The elegant part about this is that dict.update(()) doesn't change the dictionary (whereas with None this would require an extra if statement everywhere).

And this also addresses your concern that title should be overridable ... now all attributes can be overridden.

@pbodnar
Copy link
Collaborator

pbodnar commented Feb 15, 2022

I updated the PR. Whereas previously attributes were either a dictionary or a string or None, now they are just dictionary or an empty tuple. The elegant part about this is that dict.update(()) doesn't change the dictionary (whereas with None this would require an extra if statement everywhere).

And this also addresses your concern that title should be overridable ... now all attributes can be overridden.

OK, this looks quite clean, yet maybe a bit hacky with the empty tuple trick (unless this is an idiomatic way in Python?). If there is no dramatic performance penalty from this solution, I would get along with it. :)

Anyway, please see my remarks and also if you would like to add some unit tests (e.g. to cover the possibility of attributes overriding), that would be great. Thank you.

@pbodnar
Copy link
Collaborator

pbodnar commented Feb 15, 2022

Note: Don't hesitate to possibly just create a new commit this time.

@legenderrys
Copy link

legenderrys commented Dec 9, 2022

Hello, I recently had the need to include html attributes onto a rendered HTML nodes.

I would like to propose an alternative solution that will not alter existing render methods.

I opened a PR#170 that demonstrates the idea.

Primary difference is how/where html attributes are stored and accessed.
@pbodnar @not-my-profile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants