Skip to content

Implement mkdocs key from v2 config #4486

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

Merged
merged 16 commits into from
Sep 27, 2018
Merged
41 changes: 40 additions & 1 deletion readthedocs/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def __init__(self, env_config, raw_config, source_file, source_position):
def error(self, key, message, code):
"""Raise an error related to ``key``."""
source = '{file} [{pos}]'.format(
file=self.source_file,
file=os.path.relpath(self.source_file, self.base_path),
Copy link
Member Author

Choose a reason for hiding this comment

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

We were showing the full path here (/rtd/builds/...)

pos=self.source_position,
)
error_message = '{source}: {message}'.format(
Expand Down Expand Up @@ -533,6 +533,13 @@ def sphinx(self):
fail_on_warning=False,
)

@property
def mkdocs(self):
return Mkdocs(
configuration=None,
fail_on_warning=False,
)


class BuildConfigV2(BuildConfigBase):

Expand All @@ -548,6 +555,12 @@ class BuildConfigV2(BuildConfigBase):
'htmldir': 'sphinx_htmldir',
'singlehtml': 'sphinx_singlehtml',
}
builders_display = {
'mkdocs': 'MkDocs',
'sphinx': 'Sphinx Html',
'sphinx_htmldir': 'Sphinx HtmlDir',
'sphinx_singlehtml': 'Sphinx Single Page 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.

I didn't want to bring these from the db, as the config module is isolated from the other rtd code

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably -1 on maintaining this in the config module separately. Was this to ensure we could move the config module out of rtd core?

I'd say either rely on the model field choices to report messages like ".. your project is configured as Sphinx HTMLdir", or skip all mentions of the Sphinx builder. I think what I was trying to describe in an earlier review was that we should skip mentioning the exact builder entirely if this complicates things too much (which seems like it is).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that was the reason. I think we can pair haha


def validate(self):
"""
Expand All @@ -566,6 +579,8 @@ def validate(self):
self.validate_doc_types()
self._config['mkdocs'] = self.validate_mkdocs()
self._config['sphinx'] = self.validate_sphinx()
# TODO: remove later
self.validate_final_doc_type()
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to remove this later #4638 (there is a note with the commit there)

self._config['submodules'] = self.validate_submodules()

def validate_formats(self):
Expand Down Expand Up @@ -806,6 +821,30 @@ def validate_sphinx(self):

return sphinx

def validate_final_doc_type(self):
"""
Validates that the doctype is the same as the admin panel.

This a temporal validation, as the configuration file
should support per version doctype, but we need to
adapt the rtd code for that.
"""
dashboard_doctype = self.defaults.get('doctype', 'sphinx')
if self.doctype != dashboard_doctype:
error_msg = (
'Your project is configured as "{}" in your admin dashboard.'
Copy link
Contributor

Choose a reason for hiding this comment

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

The error message shouldn't end with a period, it results in two sentence fragments:

Your project is configured as "Mkdocs" in your admin dashboard. But there is no "mkdocs" key specified.

It would be better as:

... in your admin, but there is no ..

).format(self.builders_display[dashboard_doctype])

if dashboard_doctype == 'mkdocs' or not self.sphinx:
error_msg += ' But there is no "{}" key specified.'.format(
'mkdocs' if dashboard_doctype == 'mkdocs' else 'sphinx'
)
else:
error_msg += ' But your "sphinx.builder" key does not match.'
Copy link
Contributor

Choose a reason for hiding this comment

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

We're no longer catching the case where the user has no sphinx key at all. I still think this needs to be a separate check on top of checking if there is a sphinx key.

Copy link
Member Author

Choose a reason for hiding this comment

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

The check for no sphinx key is above


key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx.builder'
self.error(key, error_msg, code=INVALID_KEYS_COMBINATION)

def validate_submodules(self):
"""
Validates the submodules key.
Expand Down
60 changes: 53 additions & 7 deletions readthedocs/config/tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1316,7 +1316,10 @@ def test_sphinx_is_default_doc_type(self):
('htmldir', 'sphinx_htmldir'),
('singlehtml', 'sphinx_singlehtml')])
def test_sphinx_builder_check_valid(self, value, expected):
build = self.get_build_config({'sphinx': {'builder': value}})
build = self.get_build_config(
{'sphinx': {'builder': value}},
{'defaults': {'doctype': expected}},
)
build.validate()
assert build.sphinx.builder == expected
assert build.doctype == expected
Expand All @@ -1333,6 +1336,8 @@ def test_sphinx_builder_default(self):
build.validate()
build.sphinx.builder == 'sphinx'

@pytest.mark.skip(
'This test is not compatible with the new validation around doctype.')
def test_sphinx_builder_ignores_default(self):
build = self.get_build_config(
{},
Expand Down Expand Up @@ -1454,6 +1459,7 @@ def test_mkdocs_configuration_check_valid(self, tmpdir):
apply_fs(tmpdir, {'mkdocs.yml': ''})
build = self.get_build_config(
{'mkdocs': {'configuration': 'mkdocs.yml'}},
{'defaults': {'doctype': 'mkdocs'}},
source_file=str(tmpdir.join('readthedocs.yml')),
)
build.validate()
Expand All @@ -1472,40 +1478,80 @@ def test_mkdocs_configuration_check_invalid(self, tmpdir):
assert excinfo.value.key == 'mkdocs.configuration'

def test_mkdocs_configuration_allow_null(self):
build = self.get_build_config({'mkdocs': {'configuration': None}},)
build = self.get_build_config(
{'mkdocs': {'configuration': None}},
{'defaults': {'doctype': 'mkdocs'}},
)
build.validate()
assert build.mkdocs.configuration is None

def test_mkdocs_configuration_check_default(self):
build = self.get_build_config({'mkdocs': {}})
build = self.get_build_config(
{'mkdocs': {}},
{'defaults': {'doctype': 'mkdocs'}},
)
build.validate()
assert build.mkdocs.configuration is None

@pytest.mark.parametrize('value', [[], True, 0, {}])
def test_mkdocs_configuration_validate_type(self, value):
build = self.get_build_config({'mkdocs': {'configuration': value}},)
build = self.get_build_config(
{'mkdocs': {'configuration': value}},
{'defaults': {'doctype': 'mkdocs'}},
)
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'mkdocs.configuration'

@pytest.mark.parametrize('value', [True, False])
def test_mkdocs_fail_on_warning_check_valid(self, value):
build = self.get_build_config({'mkdocs': {'fail_on_warning': value}})
build = self.get_build_config(
{'mkdocs': {'fail_on_warning': value}},
{'defaults': {'doctype': 'mkdocs'}},
)
build.validate()
assert build.mkdocs.fail_on_warning is value

@pytest.mark.parametrize('value', [[], 'invalid', 5])
def test_mkdocs_fail_on_warning_check_invalid(self, value):
build = self.get_build_config({'mkdocs': {'fail_on_warning': value}})
build = self.get_build_config(
{'mkdocs': {'fail_on_warning': value}},
{'defaults': {'doctype': 'mkdocs'}},
)
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'mkdocs.fail_on_warning'

def test_mkdocs_fail_on_warning_check_default(self):
build = self.get_build_config({'mkdocs': {}})
build = self.get_build_config(
{'mkdocs': {}},
{'defaults': {'doctype': 'mkdocs'}},
)
build.validate()
assert build.mkdocs.fail_on_warning is False

def test_validates_different_filetype_mkdocs(self):
build = self.get_build_config(
{'mkdocs': {}},
{'defaults': {'doctype': 'sphinx'}},
)
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'mkdocs'
assert 'configured as "Sphinx Html"' in str(excinfo.value)
assert 'there is no "sphinx" key' in str(excinfo.value)

def test_validates_different_filetype_sphinx(self):
build = self.get_build_config(
{'sphinx': {}},
{'defaults': {'doctype': 'sphinx_htmldir'}},
)
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'sphinx.builder'
assert 'configured as "Sphinx HtmlDir"' in str(excinfo.value)
assert 'your "sphinx.builder" key does not match' in str(excinfo.value)

@pytest.mark.parametrize('value', [[], 'invalid', 0])
def test_submodules_check_invalid_type(self, value):
build = self.get_build_config({'submodules': value})
Expand Down
19 changes: 11 additions & 8 deletions readthedocs/doc_builder/backends/mkdocs.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,15 @@ def __init__(self, *args, **kwargs):

def get_yaml_config(self):
"""Find the ``mkdocs.yml`` file in the project root."""
# TODO: try to load from the configuration file first.
test_path = os.path.join(
self.project.checkout_path(self.version.slug),
'mkdocs.yml'
)
if os.path.exists(test_path):
return test_path
return None
mkdoc_path = self.config.mkdocs.configuration
if not mkdoc_path:
mkdoc_path = os.path.join(
self.project.checkout_path(self.version.slug),
'mkdocs.yml'
)
if not os.path.exists(mkdoc_path):
return None
return mkdoc_path

def load_yaml_config(self):
"""
Expand Down Expand Up @@ -184,6 +185,8 @@ def build(self):
'--site-dir', self.build_dir,
'--config-file', self.yaml_file,
]
if self.config.mkdocs.fail_on_warning:
build_command.append('--strict')
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I'd let users configure this in their mkdocs.yml rather than adding an extra Read the Docs option.

Copy link
Member Author

@stsewd stsewd Sep 11, 2018

Choose a reason for hiding this comment

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

I'm +1 for this, I didn't know about that, Sphinx doesn't have something like that (I mean, using the conf.py file).

Copy link
Member

Choose a reason for hiding this comment

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

I think you could define setup() function in conf.py and override config.warningiserror = True and I think it should work.

cmd_ret = self.run(
*build_command,
cwd=checkout_path,
Expand Down
6 changes: 3 additions & 3 deletions readthedocs/doc_builder/python_environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ def install_core_requirements(self):
'recommonmark==0.4.0',
]

if self.project.documentation_type == 'mkdocs':
if self.config.doctype == 'mkdocs':
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm leaving the doctype access in some places where we always use the config object, it doesn't break anything anyway. Just keeping the same pattern.

requirements.append('mkdocs==0.17.3')
else:
# We will assume semver here and only automate up to the next
Expand Down Expand Up @@ -275,7 +275,7 @@ def install_core_requirements(self):
def install_user_requirements(self):
requirements_file_path = self.config.python.requirements
if not requirements_file_path and requirements_file_path != '':
builder_class = get_builder_class(self.project.documentation_type)
builder_class = get_builder_class(self.config.doctype)
docs_dir = (builder_class(build_env=self.build_env, python_env=self)
.docs_dir())
paths = [docs_dir, '']
Expand Down Expand Up @@ -352,7 +352,7 @@ def install_core_requirements(self):
'recommonmark',
]

if self.project.documentation_type == 'mkdocs':
if self.config.doctype == 'mkdocs':
pip_requirements.append('mkdocs')
else:
pip_requirements.append('readthedocs-sphinx-ext')
Expand Down
15 changes: 5 additions & 10 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ
self.slug = slugify(self.name)
if self.slug == '':
raise Exception(_('Model must have slug'))
if self.documentation_type == 'auto':
# This used to determine the type and automatically set the
# documentation type to Sphinx for rST and Mkdocs for markdown.
# It now just forces Sphinx, due to markdown support.
self.documentation_type = 'sphinx'
super(Project, self).save(*args, **kwargs)
for owner in self.users.all():
assign('view_project', owner, self)
Expand Down Expand Up @@ -590,16 +595,6 @@ def conf_dir(self, version=LATEST):
if conf_file:
return os.path.dirname(conf_file)

@property
def is_type_sphinx(self):
"""Is project type Sphinx."""
return 'sphinx' in self.documentation_type

@property
def is_type_mkdocs(self):
"""Is project type Mkdocs."""
return 'mkdocs' in self.documentation_type

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing these is a first step to deprecate Project.doctype

@property
def is_imported(self):
return bool(self.repo)
Expand Down
Loading