Skip to content

Rework use_directory_url=True validation to better catch incorrect links#80

Merged
manuzhang merged 5 commits intomanuzhang:mainfrom
ben-foxmoore:fix-use-directory-urls
Mar 26, 2024
Merged

Rework use_directory_url=True validation to better catch incorrect links#80
manuzhang merged 5 commits intomanuzhang:mainfrom
ben-foxmoore:fix-use-directory-urls

Conversation

@ben-foxmoore
Copy link
Copy Markdown
Contributor

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 from tests/integration/mkdocs.yml

         raise_error_excludes:
           404: [
             'https://www.mkdocs.org/user-guide/*',
             '#acknowledge',
-            '../index.html#BAD_ANCHOR',
-            'page2.html#BAD_ANCHOR',
             '../../../tests',
           ]

and 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_excludes for use_directory_url: True are ../page2/#BAD_ANCHOR and ../../#BAD_ANCHOR.)


This PR adds unit tests to ensure the expected behaviour for get_url_status is achieved for use_directory_url: True - which currently fail on main - 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.

@manuzhang manuzhang merged commit d9fd2ce into manuzhang:main Mar 26, 2024
@ben-foxmoore
Copy link
Copy Markdown
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.

@manuzhang
Copy link
Copy Markdown
Owner

@ben-foxmoore I can make a patch release if you are waiting on it.

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.

2 participants