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

Fixed redirects for computed destinations and URLs #45

Merged
merged 7 commits into from
Sep 18, 2022
Merged

Fixed redirects for computed destinations and URLs #45

merged 7 commits into from
Sep 18, 2022

Conversation

squidfunk
Copy link
Contributor

@squidfunk squidfunk commented Sep 12, 2022

Fixes #46

When defining redirects for Material for MkDocs' new built-in blog plugin, I stumbled upon a problem where computed URLs are not taken into account. The blog plugin recursively enumerates all Markdown files in the folder docs/blog/posts and then rewrites the File objects to have the correct dest_path and url values in the on_files hook. This is done so MkDocs and other plugins know the correct destinations for all blog posts. The result looks like this:

File(
  src_path='blog/posts/blog-support-just-landed.md', 
  dest_path='blog/2022/09/12/blog-support-just-landed/index.html', 
  name='blog-support-just-landed', 
  url='blog/2022/09/12/blog-support-just-landed/'
)

Now, when I map the old paths to the new src_path, the redirects plugin will ignore the computed, file.url and compute the target URL by itself, resulting in blog/posts/blog-support-just-landed/, which mismatches the destination URL.

This PR fixes the problem by using the computed URL, so the following configuration is correct:

blog/2022/chinese-search-support.md: blog/posts/chinese-search-support.md
blog/2021/the-past-present-and-future.md: blog/posts/the-past-present-and-future.md
blog/2021/excluding-content-from-search.md: blog/posts/excluding-content-from-search.md
blog/2021/search-better-faster-smaller.md: blog/posts/search-better-faster-smaller.md

@squidfunk
Copy link
Contributor Author

I'm not sure whether the failing tests are false positives or not, due to the changed input of the function.

@squidfunk
Copy link
Contributor Author

squidfunk commented Sep 13, 2022

The fact that the tests only check get_relative_html_path made them fail, as this function now receives URL inputs (previously .md inputs). For this reasons, we now create the File objects that MkDocs will create, and pass them to the function. I guess there are several opportunities for refactoring since the logic should be much simpler now, but I'm not confident enough to do the refactoring as I just touched the code base for the first time.

Any thoughts on why Python 2.7 fails?

@oprypin
Copy link
Contributor

oprypin commented Sep 17, 2022

This is a great implementation!
I am able to reproduce locally with Python 2.7, looking into it.
Also I suspect we will be able to throw away a bit more code after this.

@oprypin
Copy link
Contributor

oprypin commented Sep 17, 2022

Uuh Python 2.7 fails because it's forced to install MkDocs 1.0.4 and that was prior to this very old bugfix...

And these failures are real with MkDocs 1.0.4 😬

@squidfunk
Copy link
Contributor Author

squidfunk commented Sep 17, 2022

Yeah, I'm not sure how to proceed here. Is Python 2.7 support still necessary? I'm not familiar enough with the ecosystem to judge. Could we issue a new release without Python 2.7 support, locking it into using the version before the merge?

@oprypin
Copy link
Contributor

oprypin commented Sep 17, 2022

I think I'll drop it.
And I'm also working on some change to tests so I can be more confident in them

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

I just have these style comments

mkdocs_redirects/plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
@oprypin
Copy link
Contributor

oprypin commented Sep 18, 2022

So I indeed dropped Python 2.7.

We're fine to merge this as is, just with the changes I suggested.

Afterwards I'll apply my refactoring of tests: https://github.com/mkdocs/mkdocs-redirects/compare/bb2ee628e2c064bd7ebc7d72e4426fa114ae7e1a
With these tests I am already very confident that this is good.

@squidfunk
Copy link
Contributor Author

All good, thanks for looking at this!

@oprypin oprypin merged commit 468c8a5 into mkdocs:master Sep 18, 2022
@oprypin oprypin mentioned this pull request Sep 18, 2022
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.

Computed URLs are not taken into account when resolving target URL
2 participants