Skip to content

Validate docs dir before writing custom js #5569

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

Merged
merged 5 commits into from
May 1, 2019
Merged

Validate docs dir before writing custom js #5569

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b887e96
Validate docs dir before writing custom js
saadmk11 Apr 4, 2019
6c6e4de
lint fix
saadmk11 Apr 4, 2019
3429ad0
lint fix
saadmk11 Apr 4, 2019
b5b8559
Validation moved to mkdocs.py
saadmk11 Apr 4, 2019
69bcd82
validation for docs_path
saadmk11 Apr 8, 2019
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
11 changes: 11 additions & 0 deletions readthedocs/doc_builder/backends/mkdocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,18 @@ def append_conf(self, **__):
MkDocsYAMLParseError.INVALID_DOCS_DIR_CONFIG,
)

# if user puts an invalid `docs_dir` path raise an Exception
if user_docs_dir:
checkout_path = self.project.checkout_path(self.version.slug)
possible_path = os.path.join(checkout_path, user_docs_dir)
Copy link
Member

Choose a reason for hiding this comment

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

We actually need to check if this path doesn't exists https://github.com/rtfd/readthedocs.org/blob/4fea14e86fe0aabffd73352402ddf8dbba39ff40/readthedocs/doc_builder/backends/mkdocs.py#L159-L162

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! you meant docs_path I thought you meant the docs_dir which user adds in the yaml. okay I'll Update the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, it is the one the users put in the mkdocs.yml file

Copy link
Member

Choose a reason for hiding this comment

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

But the docs dir is relative to the mkdocs.yml file, which can be located in other places not jus in the root

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd Thanks for the explanation. 👍


if not os.path.exists(possible_path):
raise MkDocsYAMLParseError(
MkDocsYAMLParseError.INVALID_DOCS_DIR_PATH,
)

docs_dir = self.docs_dir(docs_dir=user_docs_dir)

self.create_index(extension='md')
user_config['docs_dir'] = docs_dir

Expand Down
5 changes: 5 additions & 0 deletions readthedocs/doc_builder/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ class MkDocsYAMLParseError(BuildEnvironmentError):
'string with relative or absolute path.',
)

INVALID_DOCS_DIR_PATH = ugettext_noop(
'The "docs_dir" config from your MkDocs YAML config file does not '
'contain a valid path.',
)

INVALID_EXTRA_CONFIG = ugettext_noop(
'The "{config}" config from your MkDocs YAML config file has to be a '
'a list of relative paths.',
Expand Down
36 changes: 36 additions & 0 deletions readthedocs/rtd_tests/tests/test_doc_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,42 @@ def test_write_js_data_docs_dir(self, checkout_path, run, generate_rtd_data):
mkdocs_config=mock.ANY,
)

@patch('readthedocs.doc_builder.backends.mkdocs.BaseMkdocs.generate_rtd_data')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_write_js_data_on_invalid_docs_dir(self, checkout_path, generate_rtd_data):
tmpdir = tempfile.mkdtemp()
os.mkdir(os.path.join(tmpdir, 'docs'))
yaml_file = os.path.join(tmpdir, 'mkdocs.yml')
yaml.safe_dump(
{
'site_name': 'mkdocs',
'google_analytics': ['UA-1234-5', 'mkdocs.org'],
'docs_dir': 'invalid_docs_dir',
'extra_css': [
'http://readthedocs.org/static/css/badge_only.css'
],
'extra_javascript': ['readthedocs-data.js'],
},
open(yaml_file, 'w'),
)
checkout_path.return_value = tmpdir

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
config=None,
)
self.searchbuilder = MkdocsHTML(
build_env=self.build_env,
python_env=python_env,
)
with self.assertRaises(MkDocsYAMLParseError):
self.searchbuilder.append_conf()
generate_rtd_data.assert_called_with(
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need this assert

Copy link
Member Author

Choose a reason for hiding this comment

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

@stsewd then how should we check if invalid docs_path raises an error or not?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we only need that check. The second assert wouldn't happen anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes that's True. 👍 updated the PR.

docs_dir='docs',
mkdocs_config=mock.ANY,
)

@patch('readthedocs.doc_builder.base.BaseBuilder.run')
@patch('readthedocs.projects.models.Project.checkout_path')
def test_append_conf_existing_yaml_with_extra(self, checkout_path, run):
Expand Down