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

Check fragments in HTML files #1198

Merged
merged 6 commits into from
Aug 22, 2023
Merged

Check fragments in HTML files #1198

merged 6 commits into from
Aug 22, 2023

Conversation

HU90m
Copy link
Contributor

@HU90m HU90m commented Aug 4, 2023

Part of the #185 work.

This PR adds fragment checking for the second type of link in #185 (comment) .

Copy link
Member

@mre mre left a comment

Choose a reason for hiding this comment

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

Looks really nice. Especially the refactoring in fragment_checker.
I've added some comments.
Are you planning to add support in html5ever as well?

lychee-lib/src/extract/html/html5gum.rs Outdated Show resolved Hide resolved
lychee-lib/src/extract/html/html5gum.rs Show resolved Hide resolved
lychee-lib/src/extract/html/mod.rs Outdated Show resolved Hide resolved
lychee-lib/src/extract/markdown.rs Show resolved Hide resolved
lychee-lib/src/utils/fragment_checker.rs Show resolved Hide resolved
@HU90m
Copy link
Contributor Author

HU90m commented Aug 6, 2023

Are you planning to add support in html5ever as well?

What would be the benefit of doing this?

@mre
Copy link
Member

mre commented Aug 6, 2023

The main advantage would be feature-parity with html5gum. Then again, we wanted to remove html5ever anyway at some point and it shouldn't be a blocker. Just checking if the PR is done and ready for review.

@HU90m
Copy link
Contributor Author

HU90m commented Aug 8, 2023

The main advantage would be feature-parity with html5gum. Then again, we wanted to remove html5ever anyway at some point and it shouldn't be a blocker. Just checking if the PR is done and ready for review.

Ah cool, yeah I hadn't created a html5ever version because it looked like html5ever was being depreciated (and it routing the html5ever option into the file checker seemed like additional unnecessary work).

@mre mre merged commit f59aa61 into lycheeverse:master Aug 22, 2023
7 checks passed
@mre
Copy link
Member

mre commented Aug 22, 2023

At long last, I finally came around to reviewing and merging this. Apologies for the delay. The feature turned out great! Thanks for the contribution (and your patience) @HU90m. 🤩

@HU90m
Copy link
Contributor Author

HU90m commented Aug 23, 2023

No apology needed. Thanks for making it so easy to contribute!

@mre mre added the enhancement New feature or request label Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants