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

Correctly report mkdocs theme name #3920

Merged
merged 4 commits into from Apr 18, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer 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:

@@ -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]
Copy link
Member

@ericholscher ericholscher Apr 10, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Contributor Author

@davidfischer davidfischer Apr 11, 2018

Choose a reason for hiding this comment

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

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.

Loading

Copy link
Member

@ericholscher ericholscher Apr 18, 2018

Choose a reason for hiding this comment

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

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?

Loading

Copy link
Contributor Author

@davidfischer davidfischer Apr 18, 2018

Choose a reason for hiding this comment

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

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

Loading

@ericholscher
Copy link
Member

@ericholscher 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

Loading

@agjohnson agjohnson added this to the Mkdocs milestone Apr 10, 2018
@agjohnson agjohnson removed the Mkdocs label Apr 10, 2018
@davidfischer
Copy link
Contributor Author

@davidfischer 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

Loading

@agjohnson agjohnson removed this from the Mkdocs milestone Apr 11, 2018
@agjohnson agjohnson added this to the 2.4 milestone Apr 11, 2018
Copy link
Member

@ericholscher ericholscher left a comment

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

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer 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.

Loading

Copy link
Member

@humitos humitos left a comment

Looks good.

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

Loading

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

@humitos humitos Apr 18, 2018

Choose a reason for hiding this comment

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

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)

Loading

Copy link
Contributor Author

@davidfischer davidfischer Apr 18, 2018

Choose a reason for hiding this comment

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

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.

Loading

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Apr 18, 2018

Updated based on feedback...

Loading

@davidfischer davidfischer merged commit 0bf1737 into readthedocs:master Apr 18, 2018
1 check passed
Loading
theme_name = 'readthedocs'
theme_dir = mkdocs_config.get('theme_dir')
if theme_dir:
theme_name = theme_dir.rstrip('/').split('/')[-1]
Copy link
Member

@ericholscher ericholscher Apr 19, 2018

Choose a reason for hiding this comment

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

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.

Loading

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.

None yet

4 participants