Skip to content
This repository has been archived by the owner on Mar 25, 2022. It is now read-only.

Remove URL quoting from refs before passing to Sphinx #158

Merged
merged 1 commit into from Jun 14, 2019

Conversation

gibfahn
Copy link
Contributor

@gibfahn gibfahn commented May 23, 2019

Sphinx allows refs with spaces etc, and in fact autogenerates them with
the command autosectionlabel.

So if you put a:

[Link 1](<some ref>)
[Link 2](<https://foo.com/bar baz>)

Then the links will be some%20ref and https://foo.com/bar%20baz.

We want to keep the URL quoting for external references, but if we're
passing it as :any: to Sphinx we need to unquote so Sphinx can find
the correct reference, the <some ref> should map to:

:ref:`some ref`

Fixes: #155

Sphinx allows refs with spaces etc, and in fact autogenerates them with
the command [`autosectionlabel`][].

So if you put a:

```markdown
[Link 1](<some ref>)
[Link 2](<https://foo.com/bar baz>)
```

Then the links will be `some%20ref` and `https://foo.com/bar%20baz`.

We want to keep the URL quoting for external references, but if we're
passing it as `:any:` to Sphinx we need to unquote so Sphinx can find
the correct reference, the `<some ref>` should map to:

```rst
:ref:`some ref`
```

[`autosectionlabel`](https://www.sphinx-doc.org/en/master/usage/extensions/autosectionlabel.html)

Fixes: readthedocs#155
@@ -152,7 +152,7 @@ def visit_link(self, mdnode):
url_check = urlparse(destination)
if not url_check.scheme and not url_check.fragment:
wrap_node = addnodes.pending_xref(
reftarget=destination,
reftarget=unquote(destination),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only actual change.

@gibfahn
Copy link
Contributor Author

gibfahn commented Jun 14, 2019

@ericholscher sorry for the direct ping, but would you mind taking a look at this? I think it should be uncontroversial.

I've been using it (via a patch --forward in my Makefile) in a project for a while now, would be great not to have to do that any more :).


Full patch code in case anyone else needs to do the same:

Patch script:

# Generate patch with:
cp .venv/lib/python3.*/site-packages/recommonmark/parser.py parser.py

# Edit ./parser.py to apply the change here

# Generate patch
diff -u .venv/lib/python3.*/site-packages/recommonmark/parser.py parser.py > parser.py.patch

# Commit ./parser.py.patch, delete ./parser.py

# Apply patch as part of your Makefile/setup.py:
patch --forward \
  .venv/lib/python3.*/site-packages/recommonmark/parser.py \
  < parser.py.patch \
  || true

Generated patch:

--- .venv/lib/python3.7/site-packages/recommonmark/parser.py	2019-05-16 11:29:29.045060000 +0200
+++ parser.py	2019-05-23 16:21:31.172900496 +0200
@@ -11,9 +11,9 @@
 from warnings import warn
 
 if sys.version_info < (3, 0):
-    from urlparse import urlparse
+    from urlparse import urlparse, unquote
 else:
-    from urllib.parse import urlparse
+    from urllib.parse import urlparse, unquote
 
 __all__ = ['CommonMarkParser']
 
@@ -152,7 +152,7 @@
         url_check = urlparse(destination)
         if not url_check.scheme and not url_check.fragment:
             wrap_node = addnodes.pending_xref(
-                reftarget=destination,
+                reftarget=unquote(destination),
                 reftype='any',
                 refdomain=None,  # Added to enable cross-linking
                 refexplicit=True,

@ericholscher
Copy link
Member

Looks good, no worries on the direct ping 👍

@ericholscher ericholscher merged commit f748692 into readthedocs:master Jun 14, 2019
@ericholscher
Copy link
Member

I will try and get a release out here soon.

@gibfahn gibfahn deleted the fix_refs_with_spaces branch June 16, 2019 11:25
@gibfahn
Copy link
Contributor Author

gibfahn commented Jun 16, 2019

Thank you!

@gibfahn
Copy link
Contributor Author

gibfahn commented Jul 17, 2019

I will try and get a release out here soon.

@ericholscher any chance of a release? If there's anything I could do to help with said release let me know.

@ericholscher
Copy link
Member

I just shipped 0.6.0 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't reference headings in a different file
2 participants