Rework use_directory_url=True validation to better catch incorrect links#80
Merged
manuzhang merged 5 commits intomanuzhang:mainfrom Mar 26, 2024
Merged
Conversation
manuzhang
approved these changes
Mar 26, 2024
Contributor
Author
|
@SeanTAllen I should probably have tagged you in this. If you're able to, I'd be interested whether this still works for the use-case that you were originally working with in #75. Hopefully I've captured that properly in the new unit tests, but confirmation would be good. |
Owner
|
@ben-foxmoore I can make a patch release if you are waiting on it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current logic behind validation of links to local Markdown-based pages is broken when
use_directory_url: True.This was improved in #75 but there are still issues with it. No unit tests were added specifically for
use_directory_url: True, and the integration tests obscure that the expected 404 links (#BAD_ANCHOR) aren't actually found at all when this option is enabled. This can be observed by removing the links fromtests/integration/mkdocs.ymland running the test
mkdocs build --use-directory-urls. This should fail, but actually passes without any errors.(In fact, this integration test in its current form should fail without any change - the URLs which should be added to
raise_error_excludesforuse_directory_url: Trueare../page2/#BAD_ANCHORand../../#BAD_ANCHOR.)This PR adds unit tests to ensure the expected behaviour for
get_url_statusis achieved foruse_directory_url: True- which currently fail onmain- and then reworks the implementation to support this. The primary logic change is to consider the type of the source file when trying to determine whether the anchor is valid. Rather than checking that the target page ends in.html, it now checks that the source file is a Markdown file (ends in.md). If it is Markdown, only then does it check that the expected anchor exists. In all other cases, it skips validating the anchor entirely.(Conceptually it should be possible to support validating links to anchors in other types of documents, but this functionality isn't something that was previously possible either, so I haven't implemented it.)
The issues with the current logic can be demonstrated by checking out
994830b- this commit is before the logic change added in this PR - and running unit and integration tests there. The unit tests fail, and the integration tests don't detect the issue.