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

Added on_after_end_tag handler #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Nov 29, 2021

This adds an on_after_end_tag handler to address #108. Note that when I made the commit, the pre-commit hook fired and reformatted the entire codebase as part 1.56.0 standard.

I'm not sure why the code base is currently not formatted to latest cargo fmt. As such this includes a lot of changes that are completely irrelevant to the actual change I was trying to make.

If this type of change is interesting I can address the code formatting situation and ad tests and docs.

@mitsuhiko mitsuhiko requested a review from a team as a code owner November 29, 2021 11:20
@mitsuhiko
Copy link
Contributor Author

So the challenge I'm now recognizing is that because a lot of HTML tags are optional there is no guarantee that you're going to have an end of tag callback. Take for instance a list of items (<li>). The closing li tag is optional.

@jyn514
Copy link
Contributor

jyn514 commented Dec 16, 2021

Thanks for the PR! #112 fixes the formatting, can you rebase over master so the PR only has your own changes?

This adds an on_after_end_tag handler to address #108.
So the challenge I'm now recognizing is that because a lot of HTML tags are optional there is no guarantee that you're going to have an end of tag callback. Take for instance a list of items (<li>). The closing li tag is optional.

Hmm, I think it makes sense to hold off on merging this until we figure out #110.

@mitsuhiko
Copy link
Contributor Author

I rebased it. I agree that merging this without addressing #110 is not immensely useful.

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.

None yet

2 participants