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
32 changes: 31 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 Down Expand Up @@ -566,6 +573,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 +815,27 @@ 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:
key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx'
self.error(
key,
'Your project is configured as "{doctype}" in your admin '
Copy link
Contributor

Choose a reason for hiding this comment

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

doctype here is the interpolation that could be displayed as sphinx_htmldir. key in this string can only be sphinx or mkdocs.

'dashboard, but there was a "{key}" key specified.'.format(
Copy link
Contributor

Choose a reason for hiding this comment

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

One last nitpick. The error is sort of inverted i feel -- we don't actually tell the user the they need a "sphinx" key, just that they can't have a "mkdocs" key. I feel we need to be clearer here, ie:

Your project is configured as "Sphinx" in your project admin dashboard, but there is no "sphinx" key specified.

Also, for some of the odd documentation types, it looks like we're putting the slug in there, so:

Your project is configured as "sphinx_htmldir" ...

I feel like we want the verbose name instead. This shouldn't be a huge change, just using the model field to resolve the verbose name, but if it is a large change, it's not horribly important. This piece is temporary and it's probably close enough for most users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, I'm not sure about accessing the db from the config module (we don't do that there). I'm changing the error message.

doctype=dashboard_doctype,
key=key,
),
code=INVALID_KEYS_COMBINATION,
)

def validate_submodules(self):
"""
Validates the submodules key.
Expand Down
47 changes: 40 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,67 @@ 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(self):
build = self.get_build_config(
{'mkdocs': {}},
{'defaults': {'doctype': 'sphinx'}},
)
with raises(InvalidConfig) as excinfo:
build.validate()
assert excinfo.value.key == 'mkdocs'

@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
58 changes: 25 additions & 33 deletions readthedocs/projects/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,10 +470,6 @@ def run_build(self, docker, record):

# Environment used for building code, usually with Docker
with self.build_env:

if self.project.documentation_type == 'auto':
self.update_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.

I move this logic to the save method of the project model


python_env_cls = Virtualenv
if self.config.conda is not None:
self._log('Using conda')
Expand Down Expand Up @@ -589,21 +585,6 @@ def set_valid_clone(self):
self.project.has_valid_clone = True
self.version.project.has_valid_clone = True

def update_documentation_type(self):
"""
Force Sphinx for 'auto' documentation type.

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.
"""
ret = 'sphinx'
project_data = api_v2.project(self.project.pk).get()
project_data['documentation_type'] = ret
api_v2.project(self.project.pk).put(project_data)
self.project.documentation_type = ret
self.version.project.documentation_type = ret

def update_app_instances(self, html=False, localmedia=False, search=False,
pdf=False, epub=False):
"""
Expand Down Expand Up @@ -642,7 +623,10 @@ def update_app_instances(self, html=False, localmedia=False, search=False,
pdf=pdf,
epub=epub,
),
callback=sync_callback.s(version_pk=self.version.pk, commit=self.build['commit']),
callback=sync_callback.s(
version_pk=self.version.pk,
commit=self.build['commit'],
),
)

def setup_python_environment(self):
Expand Down Expand Up @@ -714,10 +698,12 @@ def build_docs_html(self):

# Gracefully attempt to move files via task on web workers.
try:
broadcast(type='app', task=move_files,
args=[self.version.pk, socket.gethostname()],
kwargs=dict(html=True)
)
broadcast(
type='app',
task=move_files,
args=[self.version.pk, socket.gethostname()],
kwargs=dict(html=True)
)
except socket.error:
log.exception('move_files task has failed on socket error.')

Expand All @@ -729,23 +715,23 @@ def build_docs_localmedia(self):
return False

if self.build_localmedia:
if self.project.is_type_sphinx:
if self.is_type_sphinx():
return self.build_docs_class('sphinx_singlehtmllocalmedia')
return False

def build_docs_pdf(self):
"""Build PDF docs."""
if ('pdf' not in self.config.formats or
self.project.slug in HTML_ONLY or
not self.project.is_type_sphinx):
self.project.slug in HTML_ONLY or
not self.is_type_sphinx()):
return False
return self.build_docs_class('sphinx_pdf')

def build_docs_epub(self):
"""Build ePub docs."""
if ('epub' not in self.config.formats or
self.project.slug in HTML_ONLY or
not self.project.is_type_sphinx):
not self.is_type_sphinx()):
return False
return self.build_docs_class('sphinx_epub')

Expand All @@ -769,6 +755,10 @@ def send_notifications(self):
"""Send notifications on build failure."""
send_notifications.delay(self.version.pk, build_pk=self.build['id'])

def is_type_sphinx(self):
"""Is documentation type Sphinx."""
return 'sphinx' in self.config.doctype


# Web tasks
@app.task(queue='web')
Expand Down Expand Up @@ -816,8 +806,8 @@ def sync_files(project_pk, version_pk, hostname=None, html=False,


@app.task(queue='web')
def move_files(version_pk, hostname, html=False, localmedia=False, search=False,
pdf=False, epub=False):
def move_files(version_pk, hostname, html=False, localmedia=False,
search=False, pdf=False, epub=False):
"""
Task to move built documentation to web servers.

Expand Down Expand Up @@ -912,11 +902,13 @@ def update_search(version_pk, commit, delete_non_commit_files=True):
"""
version = Version.objects.get(pk=version_pk)

if version.project.is_type_sphinx:
if 'sphinx' in version.project.documentation_type:
page_list = process_all_json_files(version, build_dir=False)
else:
log.debug('Unknown documentation type: %s',
version.project.documentation_type)
log.debug(
'Unknown documentation type: %s',
version.project.documentation_type
)
return

log_msg = ' '.join([page['path'] for page in page_list])
Expand Down
Loading