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.

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

Validate docs dir before writing custom js #5569

Merged
merged 5 commits into from May 1, 2019

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Apr 4, 2019

closes #5568

Copy link
Member

@stsewd stsewd left a comment

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

@stsewd stsewd Apr 4, 2019

We shouldn't put special cases in the base class, the base class should know nothing about mkdocs.

Copy link
Member Author

@saadmk11 saadmk11 Apr 4, 2019

Got it :)

Copy link
Member Author

@saadmk11 saadmk11 Apr 4, 2019

@stsewd Updated the PR

# 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

@stsewd stsewd Apr 8, 2019

Copy link
Member Author

@saadmk11 saadmk11 Apr 8, 2019

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

@stsewd stsewd Apr 8, 2019

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

Copy link
Member

@stsewd stsewd Apr 8, 2019

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

@saadmk11 saadmk11 Apr 8, 2019

@stsewd Thanks for the explanation. 👍

)
with self.assertRaises(MkDocsYAMLParseError):
self.searchbuilder.append_conf()
generate_rtd_data.assert_called_with(
Copy link
Member

@stsewd stsewd Apr 8, 2019

I guess we don't need this assert

Copy link
Member Author

@saadmk11 saadmk11 Apr 8, 2019

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

Copy link
Member

@stsewd stsewd Apr 8, 2019

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

Copy link
Member Author

@saadmk11 saadmk11 Apr 8, 2019

yes that's True. 👍 updated the PR.

stsewd
stsewd approved these changes Apr 8, 2019
Copy link
Member

@stsewd stsewd left a comment

Looks good to me, I left to someone else the review of the copy

@stsewd stsewd requested a review from Apr 8, 2019
@stsewd stsewd merged commit 0585d1d into readthedocs:master May 1, 2019
1 check passed
@saadmk11 saadmk11 deleted the validate-docs-dir branch May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants