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.

Sign up for GitHub

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

Updates and simplification for mkdocs #4556

Merged
merged 4 commits into from
Sep 12, 2018
Merged

Updates and simplification for mkdocs #4556

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a30768d
Updates and simplification for mkdocs
davidfischer Aug 22, 2018
2d446c7
Merge branch 'master' into davidfischer/mkdocs-upgrade-again
davidfischer Sep 12, 2018
171ce95
Fix a merge mistake
davidfischer Sep 12, 2018
4d82d8c
Merge branch 'master' into davidfischer/mkdocs-upgrade-again
davidfischer Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions media/css/readthedocs-doc-embed.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
display: block;

bottom: 50px;

/* Workaround for mkdocs which set a specific height for this element */
height: auto;
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 fixes the bug seen here which is currently visible on https://mkdocs.readthedocs.io/en/stable/

screen shot 2018-08-22 at 10 16 18 am

}

.rst-other-versions {
Expand Down
3 changes: 2 additions & 1 deletion readthedocs/core/static-src/core/js/doc-embed/footer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ function injectFooter(data) {

// If the theme looks like ours, update the existing badge
// otherwise throw a a full one into the page.
if (config.is_rtd_like_theme()) {
// Do not inject for mkdocs even for the RTD theme
if (config.is_sphinx_builder() && config.is_rtd_like_theme()) {
$("div.rst-other-versions").html(data['html']);
} else {
$("body").append(data['html']);
Expand Down
3 changes: 2 additions & 1 deletion readthedocs/core/static-src/core/js/doc-embed/rtd-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ var constants = require('./constants');

var configMethods = {
is_rtd_like_theme: function () {
// Returns true for the Read the Docs theme on both sphinx and mkdocs
if ($('div.rst-other-versions').length === 1) {
// Crappy heuristic, but people change the theme name
// So we have to do some duck typing.
Expand All @@ -30,7 +31,7 @@ var configMethods = {
},

is_mkdocs_builder: function () {
return (!('builder' in this) || this.builder === 'mkdocs');
return ('builder' in this && this.builder === 'mkdocs');
},

get_theme_name: function () {
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/core/static-src/core/js/doc-embed/sphinx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function init() {
/// Inject the Read the Docs Sphinx theme code
/// This is necessary on older versions of the RTD theme (<0.4.0)
/// and on themes other then the RTD theme (used for the version menu)
if ((rtd.builder === undefined || rtd.builder === 'sphinx') && window.SphinxRtdTheme === undefined) {
if (window.SphinxRtdTheme === undefined) {
sphinx_theme = require('sphinx-rtd-theme'); // eslint-disable-line global-require

var theme = sphinx_theme.ThemeNav;
Expand Down
45 changes: 4 additions & 41 deletions readthedocs/doc_builder/backends/mkdocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,8 @@ class BaseMkdocs(BaseBuilder):

"""Mkdocs builder."""

use_theme = True

# The default theme for mkdocs (outside of RTD) is the 'mkdocs' theme
# For RTD, our default is the 'readthedocs' theme
READTHEDOCS_THEME_NAME = 'readthedocs'

# Overrides for the 'readthedocs' theme that include
# search utilities and version selector
READTHEDOCS_TEMPLATE_OVERRIDE_DIR = (
'%s/readthedocs/templates/mkdocs/readthedocs' % settings.SITE_ROOT
)
# The default theme for mkdocs is the 'mkdocs' theme
DEFAULT_THEME_NAME = 'mkdocs'
Copy link
Member

Choose a reason for hiding this comment

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

Why not default to the rtd theme when users don't have a theme or theme.name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Starting with mkdocs>=1.0, theme is required. Also, this is another source of confusion where Read the Docs does something different than the default MkDocs functionality. I believe we shouldn't be changing users' intents.

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense, I wonder if this is good for old projects, all of them were building with the rtd theme, right?

Copy link
Member

Choose a reason for hiding this comment

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

I like to keep the same behavior outside than inside RTD.

I want to echo Santos' question: what would happen with old projects that weren't specifying the them? Will they start building with the mkdocs theme? If so, I suppose we should do something to avoid that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming they target mkdocs<1.0 and don't specify a theme, they will start to build with the mkdocs theme. This would be the same behavior as if they built locally. Starting with mkdocs>=1.0, they will fail to build with an error because theme is required.

Frankly, I think switching them to the mkdocs theme is the correct behavior as then their project builds with the same theme inside and outside RTD. In general, however, I suspect not a lot of projects do not specify a theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am -1 on this particular change, I think projects should keep the readthedocs theme if that is what they have been building their projects with. We can use a date-based feature flag if necessary, to ensure that projects before the feature flag get the readthedocs theme.

If the user hasn't changed to a custom theme, I don't think we can accurately determine whether they want the default mkdocs theme or our theme, so i'm more inclined to just keep our theme for these users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put some thoughts here about this. I moved the discussion a bit since it references multiple comments.


def __init__(self, *args, **kwargs):
super(BaseMkdocs, self).__init__(*args, **kwargs)
Expand Down Expand Up @@ -133,11 +124,6 @@ def append_conf(self, **__):
# This supports using RTD's privacy improvements around analytics
user_config['google_analytics'] = None

# If using the readthedocs theme, apply the readthedocs.org overrides
# These use a global readthedocs search
# and customize the version selector.
self.apply_theme_override(user_config)

# Write the modified mkdocs configuration
yaml.safe_dump(
user_config,
Expand Down Expand Up @@ -198,8 +184,6 @@ def build(self):
'--site-dir', self.build_dir,
'--config-file', self.yaml_file,
]
if self.use_theme:
build_command.extend(['--theme', 'readthedocs'])
cmd_ret = self.run(
*build_command,
cwd=checkout_path,
Expand All @@ -220,7 +204,7 @@ def get_theme_name(self, mkdocs_config):
theme_setting = mkdocs_config.get('theme')
if isinstance(theme_setting, dict):
# Full nested theme config (the new configuration)
return theme_setting.get('name') or self.READTHEDOCS_THEME_NAME
return theme_setting.get('name') or self.DEFAULT_THEME_NAME

if theme_setting:
# A string which is the name of the theme
Expand All @@ -231,27 +215,7 @@ def get_theme_name(self, mkdocs_config):
# Use the name of the directory in this project's custom theme directory
return theme_dir.rstrip('/').split('/')[-1]

return self.READTHEDOCS_THEME_NAME

def apply_theme_override(self, mkdocs_config):
"""
Apply theme overrides for the RTD theme (modifies the ``mkdocs_config`` parameter)
In v0.17.0, the theme configuration switched
from two separate configs (both optional) to a nested directive.
How to override the theme depends on whether the new or old configuration
is used.
:see: http://www.mkdocs.org/about/release-notes/#theme-customization-1164
"""
if self.get_theme_name(mkdocs_config) == self.READTHEDOCS_THEME_NAME:
# Overriding the theme is only necessary
# if the 'readthedocs' theme is used.
theme_setting = mkdocs_config.get('theme')
if isinstance(theme_setting, dict):
theme_setting['custom_dir'] = self.READTHEDOCS_TEMPLATE_OVERRIDE_DIR
else:
mkdocs_config['theme_dir'] = self.READTHEDOCS_TEMPLATE_OVERRIDE_DIR
return self.DEFAULT_THEME_NAME


class MkdocsHTML(BaseMkdocs):
Expand All @@ -264,4 +228,3 @@ class MkdocsJSON(BaseMkdocs):
type = 'mkdocs_json'
builder = 'json'
build_dir = '_build/json'
use_theme = False
98 changes: 34 additions & 64 deletions readthedocs/rtd_tests/tests/test_doc_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,40 @@ def setUp(self):
self.build_env.project = self.project
self.build_env.version = self.version

def test_get_theme_name(self):
builder = MkdocsHTML(
build_env=self.build_env,
python_env=None
)

# The default theme is mkdocs but in mkdocs>=1.0, theme is required
self.assertEqual(builder.get_theme_name({}), 'mkdocs')

# mkdocs<0.17 syntax
config = {
'theme': 'readthedocs',
}
self.assertEqual(builder.get_theme_name(config), 'readthedocs')

# mkdocs>=0.17 syntax
config = {
'theme': {
'name': 'test_theme',
},
}
self.assertEqual(builder.get_theme_name(config), 'test_theme')

# No theme but just a directory
config = {
'theme_dir': '/path/to/mydir',
}
self.assertEqual(builder.get_theme_name(config), 'mydir')
config = {
'theme_dir': '/path/to/mydir/',
}
self.assertEqual(builder.get_theme_name(config), 'mydir')


@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_append_conf_create_yaml(self, checkout_path, run):
Expand Down Expand Up @@ -225,70 +259,6 @@ def test_append_conf_existing_yaml_on_root(self, checkout_path, run):
'mkdocs'
)

@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_override_theme_new_style(self, checkout_path, run):
tmpdir = tempfile.mkdtemp()
os.mkdir(os.path.join(tmpdir, 'docs'))
yaml_file = os.path.join(tmpdir, 'mkdocs.yml')
yaml.safe_dump(
{
'theme': {
'name': 'readthedocs',
},
'site_name': 'mkdocs',
'docs_dir': 'docs',
},
open(yaml_file, 'w')
)
checkout_path.return_value = tmpdir

self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
)
self.searchbuilder.append_conf()

run.assert_called_with('cat', 'mkdocs.yml', cwd=mock.ANY)

config = yaml.safe_load(open(yaml_file))
self.assertEqual(
config['theme'],
{
'name': 'readthedocs',
'custom_dir': BaseMkdocs.READTHEDOCS_TEMPLATE_OVERRIDE_DIR
}
)

@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_override_theme_old_style(self, checkout_path, run):
tmpdir = tempfile.mkdtemp()
os.mkdir(os.path.join(tmpdir, 'docs'))
yaml_file = os.path.join(tmpdir, 'mkdocs.yml')
yaml.safe_dump(
{
'theme': 'readthedocs',
'site_name': 'mkdocs',
'docs_dir': 'docs',
},
open(yaml_file, 'w')
)
checkout_path.return_value = tmpdir

self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=None
)
self.searchbuilder.append_conf()

run.assert_called_with('cat', 'mkdocs.yml', cwd=mock.ANY)

config = yaml.safe_load(open(yaml_file))
self.assertEqual(
config['theme_dir'],
BaseMkdocs.READTHEDOCS_TEMPLATE_OVERRIDE_DIR
)

@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
Expand Down
5 changes: 0 additions & 5 deletions readthedocs/templates/mkdocs/readthedocs/searchbox.html
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions readthedocs/templates/mkdocs/readthedocs/versions.html
View file
Open in desktop

This file was deleted.