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: Don't compute relative URLs of already relative ones #15

Merged
merged 1 commit into from
Mar 6, 2022

Conversation

pawamoy
Copy link
Member

@pawamoy pawamoy commented Feb 26, 2022

In the last changes I made, I wanted to optimize things a bit as to avoid recomputing URLs again and again by interrogating handlers. But I also introduced a bug: URLs are recorded, but are always made relative. So an already relative URL would be stored, and made relative again, which made it incorrect.

We have three ways to fix this:

  1. simply don't record URLs:
   with contextlib.suppress(KeyError):
-      url = self.get_item_url(new_identifier, from_url)
-      self._url_map[identifier] = url  # update the map to avoid doing all this again
-      return url
+      return self.get_item_url(new_identifier, from_url)
  1. split the function in two so that URLs are made relative outside of the recursive function, where URLs are recorded: the change in this PR. Note that the fallback mechanism is able to return an absolute URL (from a loaded inventory). This PR keeps that behavior but maybe that's not something we want (I can't a find a use-case where a handler returns an identifier that is found in an inventory). We must check if an URL is absolute to avoid trying to make it relative.

  2. same as 2 but absolute URLs (from inventories) are never picked up when falling back. We can get rid of the "is absolute?" check:

    def _get_item_url(  # noqa: WPS234
        self,
        identifier: str,
        fallback: Optional[Callable[[str], Sequence[str]]] = None,
    ) -> str:
        try:
            return self._url_map[identifier]
        except KeyError:
            if fallback:
                new_identifiers = fallback(identifier)
                for new_identifier in new_identifiers:
                    with contextlib.suppress(KeyError):
                        url = self._get_item_url(new_identifier)
                        self._url_map[identifier] = url
                        return url
            raise

    def get_item_url(  # noqa: WPS234
        self,
        identifier: str,
        from_url: Optional[str] = None,
        fallback: Optional[Callable[[str], Sequence[str]]] = None,
    ) -> str:
        try:
            url = self._get_item_url(identifier, fallback)
        except KeyError:
            if identifier in self._abs_url_map:
                return self._abs_url_map[identifier]
            raise
        if from_url is not None:
            return relative_url(from_url, url)
        return url

@pawamoy pawamoy requested a review from oprypin February 26, 2022 23:01
@pawamoy
Copy link
Member Author

pawamoy commented Mar 6, 2022

@oprypin I intend to merge this in 2-3 days.

Copy link
Member

@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.

Seems good

src/mkdocs_autorefs/plugin.py Outdated Show resolved Hide resolved
src/mkdocs_autorefs/plugin.py Outdated Show resolved Hide resolved
Co-authored-by: Oleh Prypin <oleh@pryp.in>
@pawamoy pawamoy merged commit f6b861c into master Mar 6, 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.

2 participants