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

Update information on mkdocs build process #4508

Merged
merged 5 commits into from Aug 21, 2018

Conversation

Projects
None yet
4 participants
@AumitLeon
Contributor

AumitLeon commented Aug 12, 2018

This PR should resolve #4507. As per #4231, builds fail with mkdocs when markdown files are not in a doc or docs directory. The mkdocs.yml could either be in the root or in the docs directory, as per https://www.mkdocs.org/about/release-notes/#stricter-directory-validation and my own testing within my own project.

I was able to solve this issue by moving my mkdocs.yml and relevant markdown files to a docs directory. I also tested this with a doc directory, see this closed PR in my project. I think it makes sense to have the docs reflect this so future users know to have their files in the correct directory.

Of course, this doesn't solve the issue of builds failing when files are at the root, but users should be aware of the current caveat.

This is my first PR, so I am definitely open to feedback, comments, and questions :)

AumitLeon added some commits Aug 12, 2018

@humitos

Thanks for your contribution!

First, I'm not an expert on MkDocs. That said, I tried to find where we do this in our code and I didn't find it. I seems that we only look for mkdocs.yml at the root of the project:

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

Reading that code, it seems that we look into the project's root directory and if we don't find it we return None. Then, we create a mkdocs.yml with just one entry site_name and add the docs_dir (one of docs, doc, Doc, book if any of them exists in that order) at

docs_dir = self.docs_dir(docs_dir=user_docs_dir)

So, looking at the code it seems that we require a mkdocs.yml in the root.

I'm a little confused, tough.

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 13, 2018

Member

The recomended layout for a mkdocs project is

mkdocs.yml
docs/
    index.md

https://www.mkdocs.org/user-guide/writing-your-docs/#file-layout

So having

README
docs/
    mkdocs.yml
    docs/
         index.md

would be a little weird...

Maybe having

README
myproject/
    mkdocs.yml
    docs/
         index.md

makes more sense. So +1 to edit the docs to reflect that.

Then, we create a mkdocs.yml with just one entry site_name and add the docs_dir (one of docs, doc, Doc, book if any of them exists in that order)

What @humitos mention is maybe what the docs wanted to say.

Member

stsewd commented Aug 13, 2018

The recomended layout for a mkdocs project is

mkdocs.yml
docs/
    index.md

https://www.mkdocs.org/user-guide/writing-your-docs/#file-layout

So having

README
docs/
    mkdocs.yml
    docs/
         index.md

would be a little weird...

Maybe having

README
myproject/
    mkdocs.yml
    docs/
         index.md

makes more sense. So +1 to edit the docs to reflect that.

Then, we create a mkdocs.yml with just one entry site_name and add the docs_dir (one of docs, doc, Doc, book if any of them exists in that order)

What @humitos mention is maybe what the docs wanted to say.

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 13, 2018

Member

Well, looks like the docs already mention that we only search in the root of the project

Member

stsewd commented Aug 13, 2018

Well, looks like the docs already mention that we only search in the root of the project

@AumitLeon

This comment has been minimized.

Show comment
Hide comment
@AumitLeon

AumitLeon Aug 13, 2018

Contributor

That makes sense to me, and I just confirmed that not having mkdocs.yml in the root of the repo causes the build to generate a new mkdocs.yml: AumitLeon/module_starter_cli#35

I think the docs don't mention that .md files should live in either a docs, doc, Doc, or book directory, so I'll add that.

Contributor

AumitLeon commented Aug 13, 2018

That makes sense to me, and I just confirmed that not having mkdocs.yml in the root of the repo causes the build to generate a new mkdocs.yml: AumitLeon/module_starter_cli#35

I think the docs don't mention that .md files should live in either a docs, doc, Doc, or book directory, so I'll add that.

@AumitLeon

This comment has been minimized.

Show comment
Hide comment
@AumitLeon

AumitLeon Aug 13, 2018

Contributor

I'm not sure why the Travis build failed, I only modified the wording within build.rst to include info on where .md files should live. Any thoughts on what might have caused that?

Contributor

AumitLeon commented Aug 13, 2018

I'm not sure why the Travis build failed, I only modified the wording within build.rst to include info on where .md files should live. Any thoughts on what might have caused that?

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 13, 2018

Member

Travis failed because of time out, I just restarted the jobs

Member

stsewd commented Aug 13, 2018

Travis failed because of time out, I just restarted the jobs

Show outdated Hide outdated docs/builds.rst Outdated
@stsewd

stsewd approved these changes Aug 13, 2018

Thanks!

@humitos

I left a question.

I think it's the only needed to merge this PR.

Show outdated Hide outdated docs/builds.rst Outdated
@AumitLeon

This comment has been minimized.

Show comment
Hide comment
@AumitLeon

AumitLeon Aug 19, 2018

Contributor

Thanks for all your comments! Is there anything else I should change before this PR gets merged in?

Contributor

AumitLeon commented Aug 19, 2018

Thanks for all your comments! Is there anything else I should change before this PR gets merged in?

@stsewd

This comment has been minimized.

Show comment
Hide comment
@stsewd

stsewd Aug 20, 2018

Member

From my side, I don't think so. We only need to someone from the core team the press the merge button :). Sorry if this takes a while

Member

stsewd commented Aug 20, 2018

From my side, I don't think so. We only need to someone from the core team the press the merge button :). Sorry if this takes a while

@AumitLeon

This comment has been minimized.

Show comment
Hide comment
@AumitLeon

AumitLeon Aug 20, 2018

Contributor

No worries at all, just happy to contribute!

Contributor

AumitLeon commented Aug 20, 2018

No worries at all, just happy to contribute!

@ericholscher

Looks great, thanks!

@ericholscher ericholscher merged commit 6c0349a into rtfd:master Aug 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment