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

Implement mkdocs key from v2 config #4486

Merged
merged 16 commits into from
Sep 27, 2018
Merged

Implement mkdocs key from v2 config #4486

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c1d791e
Implement mkdocs key
stsewd Aug 7, 2018
5918ce0
Add mkdocs key to v1
stsewd Aug 7, 2018
019fc27
Unskip tests
stsewd Sep 10, 2018
8b077e9
Use config object in python env
stsewd Sep 11, 2018
050c974
Move auto detection for doctype
stsewd Sep 11, 2018
c469d64
Use config object to get doctype
stsewd Sep 11, 2018
ce16fe7
Remove mock
stsewd Sep 11, 2018
9caace0
Revert changes on documentation_type/doctype
stsewd Sep 17, 2018
d9d2f25
Fix test
stsewd Sep 17, 2018
b0d23a6
Validate same doctype (web and config file)
stsewd Sep 17, 2018
3962715
Use relative path to shown error
stsewd Sep 18, 2018
1aa8126
Better error message
stsewd Sep 19, 2018
ca60dca
Add reason on skipped tests
stsewd Sep 19, 2018
96f6241
Better error message
stsewd Sep 20, 2018
a03da2b
Better error message
stsewd Sep 21, 2018
357f034
Better error msg
stsewd Sep 26, 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
32 changes: 19 additions & 13 deletions readthedocs/config/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
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 Down Expand Up @@ -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.'
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):
"""
Expand Down
15 changes: 14 additions & 1 deletion readthedocs/config/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1530,14 +1530,27 @@ 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'}},
)
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):
Expand Down