Skip to content

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

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

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

ben-foxmoore
Copy link
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
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
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