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

Correctly report mkdocs theme name #3920

Merged
merged 4 commits into from Apr 18, 2018

Conversation

Projects
None yet
4 participants
@davidfischer
Contributor

davidfischer commented Apr 6, 2018

The vast majority of docs built with mkdocs are in fact the readthedocs theme. However, some aren't. Here are some counter-examples:

davidfischer added some commits Apr 6, 2018

@@ -100,6 +100,8 @@ def append_conf(self, **__):
if 'theme_dir' not in user_config and self.use_theme:
user_config['theme_dir'] = TEMPLATE_DIR
user_config['theme_name'] = user_config.get('theme_dir', 'readthedocs').rsplit('/')[-1]

This comment has been minimized.

@ericholscher

ericholscher Apr 10, 2018

Member

Do we actually want to set this here? This will change what we're writing to the users config, which seems like we don't want to do that.

This comment has been minimized.

@davidfischer

davidfischer Apr 11, 2018

Contributor

This user_config comes from their mkdocs.yml file. The only place it is written is to the READTHEDOCS_DATA variable in the docs. I believe we do in fact want to know if people are using mkdocs with a different theme.

This comment has been minimized.

@ericholscher

ericholscher Apr 18, 2018

Member

We dump the mkdocs.yml back out below though, so this variable will exist in the mkdocs.yml we use to build the users project. Will having this unknown YAML key not mess with stuff?

This comment has been minimized.

@davidfischer

davidfischer Apr 18, 2018

Contributor

Oh, I didn't fully realize what you meant. Ya, this should probably happen after the mkdocs.yml is written...

@ericholscher

This comment has been minimized.

Member

ericholscher commented Apr 10, 2018

I thought we were forcing the RTD theme for mkdocs. I'm curious how these folks got around that: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/backends/mkdocs.py#L154

@agjohnson agjohnson added this to the Mkdocs milestone Apr 10, 2018

@agjohnson agjohnson removed the Mkdocs label Apr 10, 2018

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Apr 11, 2018

They get around it by specifying a theme directory in their mkdocs.yml file. Here's an example: https://github.com/MinecraftForge/Documentation/blob/master/mkdocs.yml#L78

@agjohnson agjohnson modified the milestones: Mkdocs, 2.4 Apr 11, 2018

@ericholscher

Looks good, assuming mkdocs doesn't blow up with a theme_name in the YAML during build.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Apr 18, 2018

I didn't fully understand what you meant previously (although it is clear in retrospect). I made a change to not write the name to mkdocs.yml while still writing it to readthedocs-data.js.

@humitos

Looks good.

I noted a possible error depending on the value of the theme_dir

"""Generate template properties and render readthedocs-data.js."""
theme_name = mkdocs_config.get('theme_dir', 'readthedocs').rsplit('/')[-1]

This comment has been minimized.

@humitos

humitos Apr 18, 2018

Member

Since we are managing a path I think it's better to use os.path. Depending on the value of theme_dir this could fail:

>>> '/home/humitos/docs/theme'.rsplit('/')
['', 'home', 'humitos', 'docs', 'theme']
>>> '/home/humitos/docs/theme/'.rsplit('/')
['', 'home', 'humitos', 'docs', 'theme', '']
>>> 

in the second case we will endup with an empty string.

I suggest something like this:

>>> os.path.basename('/home/humitos/docs/theme/'.rstrip('/'))
'theme'
>>> os.path.basename('/home/humitos/docs/theme'.rstrip('/'))
'theme'

(you can achieve the same behaviour with your implementation just adding the rstrip --just in case)

This comment has been minimized.

@davidfischer

davidfischer Apr 18, 2018

Contributor

I hesitate to use os.path because I suspect MkDocs always uses / regardless of what os.sep is.

However, your point on rstrip is well taken. I'll update.

@davidfischer

This comment has been minimized.

Contributor

davidfischer commented Apr 18, 2018

Updated based on feedback...

@davidfischer davidfischer merged commit 0bf1737 into rtfd:master Apr 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
theme_name = 'readthedocs'
theme_dir = mkdocs_config.get('theme_dir')
if theme_dir:
theme_name = theme_dir.rstrip('/').split('/')[-1]

This comment has been minimized.

@ericholscher

ericholscher Apr 19, 2018

Member

I could also see this just being theme in practice, for example docs/custom/theme or something. Should we be slugifying the entire path name here, instead of just taking the last attribute? I guess for what we're doing it's close enough, but it does still feel lossy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment