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

Jump to bottom

Remove doctype from resolver #5230

Merged
merged 4 commits into from Feb 28, 2019
Merged

Remove doctype from resolver #5230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e4da97
Remove doctype from resolver
stsewd Feb 5, 2019
1c068f0
Fix tests
stsewd Feb 5, 2019
9874b86
Merge branch 'master' into remove-doctype-from-resolver
stsewd Feb 26, 2019
1a1c996
Merge branch 'master' into remove-doctype-from-resolver
stsewd Feb 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 2 additions & 24 deletions readthedocs/core/resolver.py
View file
Open in desktop
@@ -1,8 +1,5 @@
# -*- coding: utf-8 -*-

"""URL resolver for documentation."""

import re
from urllib.parse import urlunparse

from django.conf import settings
Expand Down Expand Up @@ -255,29 +252,10 @@ def _fix_filename(self, project, filename):
"""
Force filenames that might be HTML file paths into proper URL's.

This basically means stripping / and .html endings and then re-adding
them properly.
This basically means stripping /.
"""
# Bail out on non-html files
if '.' in filename and '.html' not in filename:
return filename
filename = filename.lstrip('/')
filename = re.sub(r'(^|/)index(?:.html)?$', '\\1', filename)
if filename:
if filename.endswith('/') or filename.endswith('.html'):
path = filename
elif project.documentation_type == 'sphinx_singlehtml':
path = 'index.html#document-' + filename
elif project.documentation_type in ['sphinx_htmldir', 'mkdocs']:
path = filename + '/'
elif '#' in filename:
# do nothing if the filename contains URL fragments
path = filename
else:
path = filename + '.html'
else:
path = ''
return path
Copy link
Member

Choose a reason for hiding this comment

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

I do feel like we introduced this for a reason, but perhaps we've removed that path from the codebase or something. I agree that the less we do with guessing URL's for things, the better, but I could still see this breaking in a random place.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, what I was thinking, this is used to reverse the url results from search, so all urls are present in the same format as the docs are build, rather than always including .html/index.html

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the only place where I can think it could break something, but when indexing from search we save the whole file, with the extension, no need to reverse anything from there. The other places we really don't use this at all.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still a little worried that this might break things, but I don't have a good way of testing it beyond just deploying it. @agjohnson do you have any memory of why we added this?

return filename

def _use_custom_domain(self, custom_domain):
"""
Expand Down
126 changes: 11 additions & 115 deletions readthedocs/rtd_tests/tests/test_core_tags.py
View file
Open in desktop
@@ -1,4 +1,3 @@
# -*- coding: utf-8 -*-
import mock
import pytest
from django.conf import settings
Expand All @@ -21,16 +20,15 @@ def setUp(self):
)

self.pip_latest_url = url_base.format(version='/en/latest/')
self.pip_latest_url_index = url_base.format(version='/en/latest/index.html')
self.pip_latest_fr_url = url_base.format(version='/fr/latest/')
self.pip_abc_url = url_base.format(version='/en/abc/')
self.pip_abc_url_index = url_base.format(version='/en/abc/index.html')
self.pip_abc_fr_url = url_base.format(version='/fr/abc/')
self.pip_abc_xyz_page_url = url_base.format(version='/en/abc/xyz.html')
self.pip_abc_xyz_fr_page_url = url_base.format(version='/fr/abc/xyz.html')
self.pip_abc_xyz_dir_url = url_base.format(version='/en/abc/xyz/')
self.pip_abc_xyz_fr_dir_url = url_base.format(version='/fr/abc/xyz/')
self.pip_abc_xyz_document = url_base.format(version='/en/abc/index.html#document-xyz')
self.pip_abc_xyz_fr_document = url_base.format(version='/fr/abc/index.html#document-xyz')
self.pip_latest_document_url = url_base.format(version='/en/latest/document/')
self.pip_abc_fr_url_index = url_base.format(version='/fr/abc/index.html')
self.pip_abc_xyz_page_url = url_base.format(version='/en/abc/xyz')
self.pip_abc_xyz_fr_page_url = url_base.format(version='/fr/abc/xyz')
self.pip_latest_document_url = url_base.format(version='/en/latest/document')
self.pip_latest_document_page_url = url_base.format(version='/en/latest/document.html')

with mock.patch('readthedocs.projects.models.broadcast'):
Expand All @@ -45,136 +43,40 @@ def test_project_only(self):
url = core_tags.make_document_url(proj, '')
self.assertEqual(url, self.pip_latest_url)

def test_project_only_htmldir(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'sphinx_htmldir'
url = core_tags.make_document_url(proj)
self.assertEqual(url, self.pip_latest_url)
url = core_tags.make_document_url(proj, '')
self.assertEqual(url, self.pip_latest_url)

def test_project_only_singlehtml(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'sphinx_singlehtml'
url = core_tags.make_document_url(proj)
self.assertEqual(url, self.pip_latest_url)
url = core_tags.make_document_url(proj, '')
self.assertEqual(url, self.pip_latest_url)

def test_translation_project_only(self):
proj = Project.objects.get(slug='pip-fr')
url = core_tags.make_document_url(proj)
self.assertEqual(url, self.pip_latest_fr_url)
url = core_tags.make_document_url(proj, '')
self.assertEqual(url, self.pip_latest_fr_url)

def test_translation_project_only_htmldir(self):
proj = Project.objects.get(slug='pip-fr')
proj.documentation_type = 'sphinx_htmldir'
url = core_tags.make_document_url(proj)
self.assertEqual(url, self.pip_latest_fr_url)
url = core_tags.make_document_url(proj, '')
self.assertEqual(url, self.pip_latest_fr_url)

def test_translation_project_only_singlehtml(self):
proj = Project.objects.get(slug='pip-fr')
proj.documentation_type = 'sphinx_singlehtml'
url = core_tags.make_document_url(proj)
self.assertEqual(url, self.pip_latest_fr_url)
url = core_tags.make_document_url(proj, '')
self.assertEqual(url, self.pip_latest_fr_url)

def test_project_and_version(self):
proj = Project.objects.get(slug='pip')
url = core_tags.make_document_url(proj, 'abc')
self.assertEqual(url, self.pip_abc_url)
url = core_tags.make_document_url(proj, 'abc', '')
self.assertEqual(url, self.pip_abc_url)

def test_project_and_version_htmldir(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'sphinx_htmldir'
url = core_tags.make_document_url(proj, 'abc')
self.assertEqual(url, self.pip_abc_url)
url = core_tags.make_document_url(proj, 'abc', '')
self.assertEqual(url, self.pip_abc_url)

def test_project_and_version_singlehtml(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'sphinx_singlehtml'
url = core_tags.make_document_url(proj, 'abc')
self.assertEqual(url, self.pip_abc_url)
url = core_tags.make_document_url(proj, 'abc', '')
self.assertEqual(url, self.pip_abc_url)

def test_translation_project_and_version(self):
proj = Project.objects.get(slug='pip-fr')
url = core_tags.make_document_url(proj, 'abc')
self.assertEqual(url, self.pip_abc_fr_url)
url = core_tags.make_document_url(proj, 'abc', '')
self.assertEqual(url, self.pip_abc_fr_url)

def test_translation_project_and_version_htmldir(self):
proj = Project.objects.get(slug='pip-fr')
proj.documentation_type = 'sphinx_htmldir'
url = core_tags.make_document_url(proj, 'abc')
self.assertEqual(url, self.pip_abc_fr_url)
url = core_tags.make_document_url(proj, 'abc', '')
self.assertEqual(url, self.pip_abc_fr_url)

def test_translation_project_and_version_singlehtml(self):
proj = Project.objects.get(slug='pip-fr')
proj.documentation_type = 'sphinx_singlehtml'
url = core_tags.make_document_url(proj, 'abc')
self.assertEqual(url, self.pip_abc_fr_url)
url = core_tags.make_document_url(proj, 'abc', '')
self.assertEqual(url, self.pip_abc_fr_url)

def test_project_and_version_and_page(self):
proj = Project.objects.get(slug='pip')
url = core_tags.make_document_url(proj, 'abc', 'xyz')
self.assertEqual(url, self.pip_abc_xyz_page_url)
url = core_tags.make_document_url(proj, 'abc', 'index')
self.assertEqual(url, self.pip_abc_url)

def test_project_and_version_and_page_htmldir(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'sphinx_htmldir'
url = core_tags.make_document_url(proj, 'abc', 'xyz')
self.assertEqual(url, self.pip_abc_xyz_dir_url)
url = core_tags.make_document_url(proj, 'abc', 'index')
self.assertEqual(url, self.pip_abc_url)

def test_project_and_version_and_page_signlehtml(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'sphinx_singlehtml'
url = core_tags.make_document_url(proj, 'abc', 'xyz')
self.assertEqual(url, self.pip_abc_xyz_document)
url = core_tags.make_document_url(proj, 'abc', 'index')
self.assertEqual(url, self.pip_abc_url)
url = core_tags.make_document_url(proj, 'abc', 'index.html')
self.assertEqual(url, self.pip_abc_url_index)

def test_translation_project_and_version_and_page(self):
proj = Project.objects.get(slug='pip-fr')
url = core_tags.make_document_url(proj, 'abc', 'xyz')
self.assertEqual(url, self.pip_abc_xyz_fr_page_url)
url = core_tags.make_document_url(proj, 'abc', 'index')
self.assertEqual(url, self.pip_abc_fr_url)

def test_translation_project_and_version_and_page_htmldir(self):
proj = Project.objects.get(slug='pip-fr')
proj.documentation_type = 'sphinx_htmldir'
url = core_tags.make_document_url(proj, 'abc', 'xyz')
self.assertEqual(url, self.pip_abc_xyz_fr_dir_url)
url = core_tags.make_document_url(proj, 'abc', 'index')
self.assertEqual(url, self.pip_abc_fr_url)

def test_translation_project_and_version_and_page_singlehtml(self):
proj = Project.objects.get(slug='pip-fr')
proj.documentation_type = 'sphinx_singlehtml'
url = core_tags.make_document_url(proj, 'abc', 'xyz')
self.assertEqual(url, self.pip_abc_xyz_fr_document)
url = core_tags.make_document_url(proj, 'abc', 'index')
self.assertEqual(url, self.pip_abc_fr_url)
url = core_tags.make_document_url(proj, 'abc', 'index.html')
self.assertEqual(url, self.pip_abc_fr_url_index)

def test_mkdocs(self):
proj = Project.objects.get(slug='pip')
Expand All @@ -188,17 +90,11 @@ def test_mkdocs_no_directory_urls(self):
url = core_tags.make_document_url(proj, LATEST, 'document.html')
self.assertEqual(url, self.pip_latest_document_page_url)

def test_mkdocs_index(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'mkdocs'
url = core_tags.make_document_url(proj, LATEST, 'index')
self.assertEqual(url, self.pip_latest_url)

def test_mkdocs_index_no_directory_urls(self):
proj = Project.objects.get(slug='pip')
proj.documentation_type = 'mkdocs'
url = core_tags.make_document_url(proj, LATEST, 'index.html')
self.assertEqual(url, self.pip_latest_url)
self.assertEqual(url, self.pip_latest_url_index)

def test_restructured_text(self):
value = '*test*'
Expand Down
5 changes: 2 additions & 3 deletions readthedocs/rtd_tests/tests/test_redirects.py
View file
Open in desktop
@@ -1,4 +1,3 @@
# -*- coding: utf-8 -*-
import logging

from django.http import Http404
Expand Down Expand Up @@ -169,7 +168,7 @@ def test_redirect_prefix_infinite(self):
project=self.pip, redirect_type='prefix',
from_url='/',
)
r = self.client.get('/redirect', HTTP_HOST='pip.readthedocs.org')
r = self.client.get('/redirect.html', HTTP_HOST='pip.readthedocs.org')
self.assertEqual(r.status_code, 302)
self.assertEqual(
r['Location'], 'http://pip.readthedocs.org/en/latest/redirect.html',
Expand Down Expand Up @@ -448,7 +447,7 @@ def test_http_filenames_return_themselves(self):
def test_redirects_no_subdomain(self):
self.assertEqual(
self.redirect.get_full_path('index.html'),
'/docs/read-the-docs/en/latest/',
'/docs/read-the-docs/en/latest/index.html',
)

@override_settings(
Expand Down