-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from 15 commits
c1d791e
5918ce0
019fc27
8b077e9
050c974
c469d64
ce16fe7
9caace0
d9d2f25
b0d23a6
3962715
1aa8126
ca60dca
96f6241
a03da2b
357f034
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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), | ||
pos=self.source_position, | ||
) | ||
error_message = '{source}: {message}'.format( | ||
|
@@ -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): | ||
|
||
|
@@ -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', | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
""" | ||
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
It would be better as:
|
||
).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.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
""" | ||
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally, I'd let users configure this in their There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could define |
||
cmd_ret = self.run( | ||
*build_command, | ||
cwd=checkout_path, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -236,7 +236,7 @@ def install_core_requirements(self): | |
'recommonmark==0.4.0', | ||
] | ||
|
||
if self.project.documentation_type == 'mkdocs': | ||
if self.config.doctype == 'mkdocs': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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, ''] | ||
|
@@ -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') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
There was a problem hiding this comment.
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/...)