From c1d791e36a7749553b9862c6603e7da2a73926f2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Aug 2018 11:48:13 -0500 Subject: [PATCH 01/16] Implement mkdocs key --- readthedocs/doc_builder/backends/mkdocs.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/readthedocs/doc_builder/backends/mkdocs.py b/readthedocs/doc_builder/backends/mkdocs.py index 2da0b9ee8d1..a211c115dac 100644 --- a/readthedocs/doc_builder/backends/mkdocs.py +++ b/readthedocs/doc_builder/backends/mkdocs.py @@ -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') cmd_ret = self.run( *build_command, cwd=checkout_path, From 5918ce0e86af0faff29ad7cd65a2329e43ebaad2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 7 Aug 2018 11:58:06 -0500 Subject: [PATCH 02/16] Add mkdocs key to v1 --- readthedocs/config/config.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 251c786ec82..66a4cf7e76a 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -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): From 019fc271fc6cd2ade715f563928cc729c2240985 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Sep 2018 13:01:12 -0500 Subject: [PATCH 03/16] Unskip tests --- readthedocs/rtd_tests/tests/test_config_integration.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index 3387c24dae9..d31deab4bef 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -752,7 +752,6 @@ def test_sphinx_fail_on_warning( append_conf.assert_called_once() move.assert_called_once() - @pytest.mark.skip @patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.move') @patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.append_conf') @patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.run') @@ -791,7 +790,6 @@ def test_mkdocs_configuration( append_conf.assert_called_once() move.assert_called_once() - @pytest.mark.skip @patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.move') @patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.append_conf') @patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.run') From 8b077e9133471cc96aca932e52993c4125fb0bca Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Sep 2018 23:38:22 -0500 Subject: [PATCH 04/16] Use config object in python env --- readthedocs/doc_builder/python_environments.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 1c79c8bd9f3..698eecc0b3a 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -236,7 +236,7 @@ def install_core_requirements(self): 'recommonmark==0.4.0', ] - if self.project.documentation_type == 'mkdocs': + if self.config.doctype == 'mkdocs': 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') From 050c974a695dccdea909fe1261d611a79a31dad5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Sep 2018 23:53:49 -0500 Subject: [PATCH 05/16] Move auto detection for doctype --- readthedocs/projects/models.py | 5 +++++ readthedocs/projects/tasks.py | 19 ------------------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 9cfb0b92c45..33208a94507 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -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) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index d5db9297c32..5ddb04d29e0 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -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() - python_env_cls = Virtualenv if self.config.conda is not None: self._log('Using conda') @@ -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): """ From c469d6403dbfc82432f721bb850f884a09aecb2a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Sep 2018 23:58:56 -0500 Subject: [PATCH 06/16] Use config object to get doctype --- readthedocs/projects/models.py | 10 -------- readthedocs/projects/tasks.py | 45 +++++++++++++++++++++------------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 33208a94507..bcf60d5a58a 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -595,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 - @property def is_imported(self): return bool(self.repo) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 5ddb04d29e0..2b04b29d2f8 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -614,6 +614,7 @@ def update_app_instances(self, html=False, localmedia=False, search=False, args=[ self.project.pk, self.version.pk, + self.config, ], kwargs=dict( hostname=socket.gethostname(), @@ -695,10 +696,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(), self.config], + kwargs=dict(html=True) + ) except socket.error: log.exception('move_files task has failed on socket error.') @@ -710,15 +713,15 @@ 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') @@ -726,7 +729,7 @@ 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') @@ -750,16 +753,22 @@ 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') -def sync_files(project_pk, version_pk, hostname=None, html=False, +def sync_files(project_pk, version_pk, config, hostname=None, html=False, localmedia=False, search=False, pdf=False, epub=False): """ Sync build artifacts to application instances. This task broadcasts from a build instance on build completion and performs synchronization of build artifacts on each application instance. + + :param config: A `readthedocs.config.BuildConfigBase` object """ # Clean up unused artifacts version = Version.objects.get(pk=version_pk) @@ -782,6 +791,7 @@ def sync_files(project_pk, version_pk, hostname=None, html=False, move_files( version_pk, hostname, + config, html=html, localmedia=localmedia, search=search, @@ -797,13 +807,14 @@ 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, config, html=False, localmedia=False, + search=False, pdf=False, epub=False): """ Task to move built documentation to web servers. :param version_pk: Version id to sync files for :param hostname: Hostname to sync to + :param config: A `readthedocs.config.BuildConfigBase` object :param html: Sync HTML :type html: bool :param localmedia: Sync local media files @@ -827,12 +838,12 @@ def move_files(version_pk, hostname, html=False, localmedia=False, search=False, if html: from_path = version.project.artifact_path( version=version.slug, - type_=version.project.documentation_type, + type_=config.doctype, ) target = version.project.rtd_build_path(version.slug) Syncer.copy(from_path, target, host=hostname) - if 'sphinx' in version.project.documentation_type: + if 'sphinx' in config.doctype: if search: from_path = version.project.artifact_path( version=version.slug, @@ -883,21 +894,21 @@ def move_files(version_pk, hostname, html=False, localmedia=False, search=False, @app.task(queue='web') -def update_search(version_pk, commit, delete_non_commit_files=True): +def update_search(version_pk, commit, config, delete_non_commit_files=True): """ Task to update search indexes. :param version_pk: Version id to update :param commit: Commit that updated index + :param config: A `readthedocs.config.BuildConfigBase` object :param delete_non_commit_files: Delete files not in commit from index """ version = Version.objects.get(pk=version_pk) - if version.project.is_type_sphinx: + if 'sphinx' in config.doctype: 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', config.doctype) return log_msg = ' '.join([page['path'] for page in page_list]) From ce16fe76d07f5ad83d6607c65dc590d5b4e0ff29 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 11 Sep 2018 00:15:07 -0500 Subject: [PATCH 07/16] Remove mock --- readthedocs/rtd_tests/tests/test_config_integration.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index d31deab4bef..5e851dd873f 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -345,7 +345,6 @@ def test_build_formats_only_pdf( assert not outcomes['localmedia'] assert not outcomes['epub'] - @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.update_documentation_type', new=MagicMock()) @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.setup_python_environment', new=MagicMock()) @patch('readthedocs.projects.tasks.UpdateDocsTaskStep.build_docs', new=MagicMock()) @patch('readthedocs.doc_builder.environments.BuildEnvironment.failed', new_callable=PropertyMock) From 9caace00dcc98ad6ff22bc8a9ca6be6b4897cd92 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 17 Sep 2018 12:24:56 -0500 Subject: [PATCH 08/16] Revert changes on documentation_type/doctype --- readthedocs/projects/tasks.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 2b04b29d2f8..c8ed9d1f2f5 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -614,7 +614,6 @@ def update_app_instances(self, html=False, localmedia=False, search=False, args=[ self.project.pk, self.version.pk, - self.config, ], kwargs=dict( hostname=socket.gethostname(), @@ -624,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): @@ -699,7 +701,7 @@ def build_docs_html(self): broadcast( type='app', task=move_files, - args=[self.version.pk, socket.gethostname(), self.config], + args=[self.version.pk, socket.gethostname()], kwargs=dict(html=True) ) except socket.error: @@ -760,15 +762,13 @@ def is_type_sphinx(self): # Web tasks @app.task(queue='web') -def sync_files(project_pk, version_pk, config, hostname=None, html=False, +def sync_files(project_pk, version_pk, hostname=None, html=False, localmedia=False, search=False, pdf=False, epub=False): """ Sync build artifacts to application instances. This task broadcasts from a build instance on build completion and performs synchronization of build artifacts on each application instance. - - :param config: A `readthedocs.config.BuildConfigBase` object """ # Clean up unused artifacts version = Version.objects.get(pk=version_pk) @@ -791,7 +791,6 @@ def sync_files(project_pk, version_pk, config, hostname=None, html=False, move_files( version_pk, hostname, - config, html=html, localmedia=localmedia, search=search, @@ -807,14 +806,13 @@ def sync_files(project_pk, version_pk, config, hostname=None, html=False, @app.task(queue='web') -def move_files(version_pk, hostname, config, html=False, localmedia=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. :param version_pk: Version id to sync files for :param hostname: Hostname to sync to - :param config: A `readthedocs.config.BuildConfigBase` object :param html: Sync HTML :type html: bool :param localmedia: Sync local media files @@ -838,12 +836,12 @@ def move_files(version_pk, hostname, config, html=False, localmedia=False, if html: from_path = version.project.artifact_path( version=version.slug, - type_=config.doctype, + type_=version.project.documentation_type, ) target = version.project.rtd_build_path(version.slug) Syncer.copy(from_path, target, host=hostname) - if 'sphinx' in config.doctype: + if 'sphinx' in version.project.documentation_type: if search: from_path = version.project.artifact_path( version=version.slug, @@ -894,21 +892,23 @@ def move_files(version_pk, hostname, config, html=False, localmedia=False, @app.task(queue='web') -def update_search(version_pk, commit, config, delete_non_commit_files=True): +def update_search(version_pk, commit, delete_non_commit_files=True): """ Task to update search indexes. :param version_pk: Version id to update :param commit: Commit that updated index - :param config: A `readthedocs.config.BuildConfigBase` object :param delete_non_commit_files: Delete files not in commit from index """ version = Version.objects.get(pk=version_pk) - if 'sphinx' in config.doctype: + if 'sphinx' in version.project.documentation_type: page_list = process_all_json_files(version, build_dir=False) else: - log.debug('Unknown documentation type: %s', config.doctype) + log.debug( + 'Unknown documentation type: %s', + version.project.documentation_type + ) return log_msg = ' '.join([page['path'] for page in page_list]) From d9d2f25c4fa3211a323436293b58ac324e6f5e68 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 17 Sep 2018 12:56:50 -0500 Subject: [PATCH 09/16] Fix test --- readthedocs/rtd_tests/tests/test_doc_builder.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_builder.py b/readthedocs/rtd_tests/tests/test_doc_builder.py index b07668f1084..a691054c708 100644 --- a/readthedocs/rtd_tests/tests/test_doc_builder.py +++ b/readthedocs/rtd_tests/tests/test_doc_builder.py @@ -150,10 +150,18 @@ def setUp(self): self.build_env.project = self.project self.build_env.version = self.version - def test_get_theme_name(self): + @patch('readthedocs.projects.models.Project.checkout_path') + def test_get_theme_name(self, checkout_path): + tmpdir = tempfile.mkdtemp() + checkout_path.return_value = tmpdir + python_env = Virtualenv( + version=self.version, + build_env=self.build_env, + config=None, + ) builder = MkdocsHTML( build_env=self.build_env, - python_env=None + python_env=python_env, ) # The default theme is mkdocs but in mkdocs>=1.0, theme is required From b0d23a6051cc5f96030926d5bce423e5fb75dc2b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 17 Sep 2018 14:20:09 -0500 Subject: [PATCH 10/16] Validate same doctype (web and config file) --- readthedocs/config/config.py | 19 ++++++++ readthedocs/config/tests/test_config.py | 46 ++++++++++++++++--- .../tests/test_config_integration.py | 7 ++- .../rtd_tests/tests/test_doc_builder.py | 2 +- 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 66a4cf7e76a..3bab6c3abd7 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -573,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() self._config['submodules'] = self.validate_submodules() def validate_formats(self): @@ -813,6 +815,23 @@ 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. + """ + if self.doctype != self.defaults.get('doctype', 'sphinx'): + key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx' + self.error( + key, + 'Your documentation type should match ' + 'the one from the admin panel of your project.', + code=INVALID_KEYS_COMBINATION, + ) + def validate_submodules(self): """ Validates the submodules key. diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 3b502af44b4..1f860b23bab 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -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 @@ -1333,6 +1336,7 @@ def test_sphinx_builder_default(self): build.validate() build.sphinx.builder == 'sphinx' + @pytest.mark.skip def test_sphinx_builder_ignores_default(self): build = self.get_build_config( {}, @@ -1454,6 +1458,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() @@ -1472,40 +1477,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}) diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index 5e851dd873f..515ff3ed9c6 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -597,7 +597,7 @@ def test_sphinx_builder( checkout_path.return_value = str(tmpdir) self.create_config_file(tmpdir, {'sphinx': {'builder': value}}) - self.project.documentation_type = 'mkdocs' + self.project.documentation_type = result self.project.save() update_docs = self.get_update_docs_task() @@ -605,6 +605,7 @@ def test_sphinx_builder( get_builder_class.assert_called_with(result) + @pytest.mark.skip @patch('readthedocs.projects.tasks.get_builder_class') def test_sphinx_builder_default( self, get_builder_class, checkout_path, tmpdir): @@ -771,6 +772,8 @@ def test_mkdocs_configuration( }, } ) + self.project.documentation_type = 'mkdocs' + self.project.save() update_docs = self.get_update_docs_task() config = update_docs.config @@ -809,6 +812,8 @@ def test_mkdocs_fail_on_warning( }, } ) + self.project.documentation_type = 'mkdocs' + self.project.save() update_docs = self.get_update_docs_task() config = update_docs.config diff --git a/readthedocs/rtd_tests/tests/test_doc_builder.py b/readthedocs/rtd_tests/tests/test_doc_builder.py index a691054c708..5879be90b05 100644 --- a/readthedocs/rtd_tests/tests/test_doc_builder.py +++ b/readthedocs/rtd_tests/tests/test_doc_builder.py @@ -16,7 +16,7 @@ from mock import patch from readthedocs.builds.models import Version -from readthedocs.doc_builder.backends.mkdocs import BaseMkdocs, MkdocsHTML +from readthedocs.doc_builder.backends.mkdocs import MkdocsHTML from readthedocs.doc_builder.backends.sphinx import BaseSphinx from readthedocs.doc_builder.python_environments import Virtualenv from readthedocs.projects.exceptions import ProjectConfigurationError From 39627154218d2bddaccba93deaccc88562961d2a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 18 Sep 2018 13:16:54 -0500 Subject: [PATCH 11/16] Use relative path to shown error --- readthedocs/config/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 3bab6c3abd7..0ec325296b1 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -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( From 1aa8126b59639545d976de38ef135e18d4cef208 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 19 Sep 2018 11:04:27 -0500 Subject: [PATCH 12/16] Better error message --- readthedocs/config/config.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 0ec325296b1..210bab322a0 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -823,12 +823,16 @@ def validate_final_doc_type(self): should support per version doctype, but we need to adapt the rtd code for that. """ - if self.doctype != self.defaults.get('doctype', 'sphinx'): + dashboard_doctype = self.defaults.get('doctype', 'sphinx') + if self.doctype != dashboard_doctype: key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx' self.error( key, - 'Your documentation type should match ' - 'the one from the admin panel of your project.', + 'Your project is configured as "{doctype}" in your admin ' + 'dashboard, but there was a "{key}" key specified.'.format( + doctype=dashboard_doctype, + key=key, + ), code=INVALID_KEYS_COMBINATION, ) From ca60dcacb96f66ba6c5b6d4a2f30f62c4a37d63b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 19 Sep 2018 11:04:52 -0500 Subject: [PATCH 13/16] Add reason on skipped tests --- readthedocs/config/tests/test_config.py | 3 ++- readthedocs/rtd_tests/tests/test_config_integration.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 1f860b23bab..2f17541047e 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -1336,7 +1336,8 @@ def test_sphinx_builder_default(self): build.validate() build.sphinx.builder == 'sphinx' - @pytest.mark.skip + @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( {}, diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index 515ff3ed9c6..edd2d20595f 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -605,7 +605,8 @@ def test_sphinx_builder( get_builder_class.assert_called_with(result) - @pytest.mark.skip + @pytest.mark.skip( + 'This test is not compatible with the new validation around doctype.') @patch('readthedocs.projects.tasks.get_builder_class') def test_sphinx_builder_default( self, get_builder_class, checkout_path, tmpdir): From 96f62419991b900192a0f5425f88096689f59835 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 20 Sep 2018 11:07:40 -0500 Subject: [PATCH 14/16] Better error message --- readthedocs/config/config.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 210bab322a0..8317f4eb819 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -826,12 +826,15 @@ def validate_final_doc_type(self): dashboard_doctype = self.defaults.get('doctype', 'sphinx') if self.doctype != dashboard_doctype: key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx' + needed_doctype = ( + 'mkdocs' if dashboard_doctype == 'mkdocs' else 'sphinx' + ) self.error( key, 'Your project is configured as "{doctype}" in your admin ' - 'dashboard, but there was a "{key}" key specified.'.format( + 'dashboard, but there is no "{key}" key specified.'.format( doctype=dashboard_doctype, - key=key, + key=needed_doctype, ), code=INVALID_KEYS_COMBINATION, ) From a03da2b1ef705a2dd0cff5afe62b8e7260d0a818 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 21 Sep 2018 13:17:01 -0500 Subject: [PATCH 15/16] Better error message --- readthedocs/config/config.py | 32 +++++++++++++++---------- readthedocs/config/tests/test_config.py | 15 +++++++++++- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 8317f4eb819..41a4a0c36d3 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -555,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', + } def validate(self): """ @@ -825,19 +831,19 @@ def validate_final_doc_type(self): """ dashboard_doctype = self.defaults.get('doctype', 'sphinx') if self.doctype != dashboard_doctype: - key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx' - needed_doctype = ( - 'mkdocs' if dashboard_doctype == 'mkdocs' else 'sphinx' - ) - self.error( - key, - 'Your project is configured as "{doctype}" in your admin ' - 'dashboard, but there is no "{key}" key specified.'.format( - doctype=dashboard_doctype, - key=needed_doctype, - ), - code=INVALID_KEYS_COMBINATION, - ) + error_msg = ( + 'Your project is configured as "{}" in your admin dashboard.' + ).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.' + + key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx.builder' + self.error(key, error_msg, code=INVALID_KEYS_COMBINATION) def validate_submodules(self): """ diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 2f17541047e..72c8100c8d2 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -1530,7 +1530,7 @@ def test_mkdocs_fail_on_warning_check_default(self): build.validate() assert build.mkdocs.fail_on_warning is False - def test_validates_different_filetype(self): + def test_validates_different_filetype_mkdocs(self): build = self.get_build_config( {'mkdocs': {}}, {'defaults': {'doctype': 'sphinx'}}, @@ -1538,6 +1538,19 @@ def test_validates_different_filetype(self): 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): From 357f034a7731fa5ae9c809bf9443faac8970d797 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 26 Sep 2018 11:35:58 -0500 Subject: [PATCH 16/16] Better error msg --- readthedocs/config/config.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 41a4a0c36d3..5d6cd1bedd2 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -11,13 +11,22 @@ import six +from readthedocs.projects.constants import DOCUMENTATION_CHOICES + from .find import find_one from .models import Build, Conda, Mkdocs, Python, Sphinx, Submodules from .parser import ParseError, parse from .validation import ( - ValidationError, validate_bool, validate_choice, validate_dict, - validate_directory, validate_file, validate_list, validate_string, - validate_value_exists) + ValidationError, + validate_bool, + validate_choice, + validate_dict, + validate_directory, + validate_file, + validate_list, + validate_string, + validate_value_exists, +) __all__ = ( 'ALL', @@ -555,12 +564,7 @@ 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', - } + builders_display = dict(DOCUMENTATION_CHOICES) def validate(self): """ @@ -832,15 +836,15 @@ def validate_final_doc_type(self): dashboard_doctype = self.defaults.get('doctype', 'sphinx') if self.doctype != dashboard_doctype: error_msg = ( - 'Your project is configured as "{}" in your admin dashboard.' + 'Your project is configured as "{}" in your admin dashboard,' ).format(self.builders_display[dashboard_doctype]) if dashboard_doctype == 'mkdocs' or not self.sphinx: - error_msg += ' But there is no "{}" key specified.'.format( + 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.' + error_msg += ' but your "sphinx.builder" key does not match.' key = 'mkdocs' if self.doctype == 'mkdocs' else 'sphinx.builder' self.error(key, error_msg, code=INVALID_KEYS_COMBINATION)