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

fix: do not fail on empty # and #top fragments #1609

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Jan 8, 2025

The empty "#" and "#top" fragments are always valid without related HTML element. Browser will scroll to the top of the page. Hence lychee must not fail on those.

Fixed #1599

@MichaIng MichaIng requested a review from mre January 8, 2025 07:05
@MichaIng MichaIng added bug Something isn't working false-positive labels Jan 8, 2025
@MichaIng MichaIng force-pushed the fix-empty+top-fragments branch from 21b9188 to bd21857 Compare January 8, 2025 07:10
@MichaIng
Copy link
Member Author

MichaIng commented Jan 8, 2025

Went with the two == first, but lint suggests is_empty(). The two test runs show the currently inconsistent results:

  1. https://github.com/lycheeverse/lychee/actions/runs/12665712696/job/35295837656
    #top in file.html succeeds, # in file.html and # + #top in file1.md fail.
  2. https://github.com/lycheeverse/lychee/actions/runs/12665772582/job/35296001037
    # in file.html succeeds, #top in file.html and # + #top in file1.md fail.

On my local tests, randomly, at least one, up to all of those failed. Would be interesting to know where this inconsistency is coming from, as I would suggest the same fragment to succeed or fail every time the same way 🤔.

@thiru-appitap
Copy link

updated my findings, the resolution and test information in here

please let me know if this works.

@MichaIng MichaIng force-pushed the fix-empty+top-fragments branch 4 times, most recently from 2a46b4d to 4158f2a Compare January 13, 2025 06:56
@MichaIng
Copy link
Member Author

Awesome, the embarrassingly missed empty/top fragment handling in case of existing cache entry was the solution 👍. The redundant map_or is unrelated to this PR.

@MichaIng MichaIng force-pushed the fix-empty+top-fragments branch 7 times, most recently from 3cac5a2 to 17bf32c Compare January 13, 2025 07:45
@mre
Copy link
Member

mre commented Feb 5, 2025

@MichaIng, sorry, I dropped the ball on that one. What's the current status on this?
I can see some lint issues, but they seem to be unrelated. Anything else or is this ready to be merged?

@MichaIng
Copy link
Member Author

MichaIng commented Feb 5, 2025

@mre
It is ready to go. The map_or redundancy annotation of clippy is unrelated and probably just due to a clippy update: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_map_or
So

.map_or(false, ...

to

.is_some_and(...

seems sane. But in a dedicated PR I suggest.

@mre
Copy link
Member

mre commented Feb 6, 2025

@MichaIng, can you rebase on top of master? I just fixed the clippy lint issues (#1625). That should make the build green.

The empty "#" and "#top" fragments are always valid without related HTML element. Browser will scroll to the top of the page. Hence lychee must not fail on those.

Credits go to @thiru-appitap for initial attempt and helping to find missing parts of the implementation.

Solves: lycheeverse#1599

Signed-off-by: MichaIng <[email protected]>
@MichaIng MichaIng force-pushed the fix-empty+top-fragments branch from 17bf32c to 7b15a2c Compare February 6, 2025 13:57
@mre mre merged commit d3d7f6a into lycheeverse:master Feb 6, 2025
6 checks passed
@mre
Copy link
Member

mre commented Feb 6, 2025

Thanks to you both, @thiru-appitap and @MichaIng. 🎉

@mre mre mentioned this pull request Feb 6, 2025
@MichaIng MichaIng deleted the fix-empty+top-fragments branch February 6, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working false-positive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Always succeed empty fragment checks
3 participants