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

Move builtin themes to external packages other than MkDocs and ReadTheDocs #530

Merged
merged 23 commits into from
Jun 27, 2015

Conversation

d0ugal
Copy link
Member

@d0ugal d0ugal commented May 16, 2015

This needs a few remaining things done:


for theme in iter_entry_points(group='mkdocs.themes', name=None):
theme = theme.load()
location = os.path.dirname(theme.__file__)
Copy link
Member

Choose a reason for hiding this comment

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

I like this. The theme author does not need to write any Python code. In sphinx, the theme author needs to write some code in the __init__.py file which returns the location. This is much better. Just include the empty file at the root of the theme, and it becomes available.

@d0ugal
Copy link
Member Author

d0ugal commented May 18, 2015

I'm not sure what happens if two packages provide the same theme name, I can't find any documentation on this and you can't specify a duplicate in one package, so I guess I'll need to manually test and see.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling b3693b2 on d0ugal:package-themes into 93a181a on mkdocs:master.

@waylan
Copy link
Member

waylan commented May 18, 2015

I'm not sure what happens if two packages provide the same theme name, I can't find any documentation on this and you can't specify a duplicate in one package, so I guess I'll need to manually test and see.

Yeah, I'm not sure either. Although my understanding is that there is no restriction on duplicates. The duplicates are separated by the namespace/package that they ship in (the idea being that an entry_point named "foo" for package A would not be confused with an entry_point named "foo" for package B. Of course, when package B is a third party addon to package A, then package A can't make that distinction. As the only way to get entry_points across multiple packages is to iterate, the last entry_point named "foo" in your for loop will be the one that gets used. I am not sure what determined the order. If it is 'last installed' that is probably fine, but it my not be and I haven't worked that part out.

@d0ugal
Copy link
Member Author

d0ugal commented May 18, 2015

Last installed would still be vague. Maybe we should change the theme config option to {pageage}.{theme_name}.

So in the case of the builtin themes, it would be mkdocs.readthedocs and mkdocs.mkdocs. I suspect many themes will end up being custom_theme.custom_theme which is annoying.

I'm tempted to say that the risk is really small enough that it is worth the convenience.

@waylan
Copy link
Member

waylan commented May 18, 2015

I think the last paragraph in this section is relevant. Specifically, it is up to you to decide how to handle duplicates. That said, the docs for iter_entry_points states that duplicates are ordered by the package/distribution they are associated with on the PYTHONPATH.

So, if two themes exist with the same name, the second one to be found on the PYTHONPATH will always overwrite the first one in your loop. Which comes first or second depends on how each machine is configured and where each package in installed on the path. You will even find different configurations on different machines using the same version of the same OS. In other words, it is completely unpredictable.

You could check for duplicates and either raise a warning or an error. At least that way a user would have a clue as to why the wrong theme is being used. Just make sure each warning/error message lists which package the duplicates are associated with so they can uninstall the one they don't want.

For the themes included with mkdocs, you can special case them to not allow duplicates. That way, others will not even bother making duplicates of the builtin themes as it will never work. Doing iter_entry_points(group='mkdocs.themes', name='mkdocs') should give you only the themes that ship with mkdocs.

@waylan
Copy link
Member

waylan commented May 18, 2015

Doing iter_entry_points(group='mkdocs.themes', name='mkdocs') should give you only the themes that ship with mkdocs.

Sorry, actually get_entry_map(dist='mkdocs', group='mkdocs.themes') will give you the themes that ship with mkdocs.

@waylan
Copy link
Member

waylan commented May 18, 2015

Perhaps something like this would work (untested):

themes = {}
builtins = get_entry_map(dist='mkdocs', group='mkdocs.themes')

for theme in iter_entry_points(group='mkdocs.themes'):
    if theme.name in builtins and theme.dist.key != 'mkdocs':
        # raise an error...
    if theme.name in themes:
        # raise a warning...
    themes[theme.name] = os.path.dirname(theme.load().__file__)

@waylan
Copy link
Member

waylan commented May 18, 2015

Another possibility (again untested):

themes = {}
for theme in iter_entry_points(group='mkdocs.themes'):
    name = '.'.join(theme.dist.key, theme.name)
    themes[name] = os.path.dirname(theme.load().__file__)

According to the docs, theme.dist.key is the unique lowercase name used to register a distribution with PyPI. So if the entry point for the builitin theme was readthedocs, then the name entered in the config file by the user would be mkdocs.readthedocs. And if I created a new separate distribution (named foo) with a readthedocs entry point, I would need to use foo.readthedocs in my config to get it to work.

There would be no duplicates. However, the entry point would not be the full name, only the last part and that could be slightly confusing to some. Clear documentation would be required.

@waylan
Copy link
Member

waylan commented May 18, 2015

And I just realized my last suggestion was the same as yours @d0ugal.

So in the case of the builtin themes, it would be mkdocs.readthedocs and mkdocs.mkdocs. I suspect many themes will end up being custom_theme.custom_theme which is annoying.

That is a valid point. But what if you renamed the "mkdocs" theme to "default" or "basic" (or something better). That way, in the config the user would set theme: mkdocs.default. Even warn theme authors in the docs about the possible name duplication. With mkdocs setting the precedent, a package with one theme would more likely follow the same pattern. And naturally, a package with multiple themes would just give a name to each one.

@d0ugal
Copy link
Member Author

d0ugal commented May 18, 2015

Thanks for throwing those ideas in, I am inclined to go for erroring if a builtin theme is used and logging a warning if a name is used twice. Simply because this provides us the easiest transition path, all the current names are will work and I expect the most common case will be theme packages that contain one theme. We can then encourage users use the package name for the theme name, ensuring some uniqueness (assuming they register and upload to PyPI).

@waylan
Copy link
Member

waylan commented May 18, 2015

@d0ugal that sounds like a reasonable conclusion and my preference as well.

@d0ugal
Copy link
Member Author

d0ugal commented May 18, 2015

It also means I can probably remove the bootswatch themes for 0.13, and just add them as a requirement. It might delay the release a bit, but I've been itching to do this for a long time.

I'm even slightly tempted to move the mkdocs and readthedocs themes into their own packages. However, that might be going a bit far. I'll need to think about that more.

@facelessuser
Copy link
Contributor

Nice! I think this is a good idea.

@d0ugal d0ugal force-pushed the package-themes branch 3 times, most recently from 7f6ec8d to 17a2e6f Compare May 21, 2015 05:57
@d0ugal d0ugal added this to the 0.14.0 milestone May 21, 2015
@d0ugal
Copy link
Member Author

d0ugal commented May 21, 2015

I think I'm happy with this, but I want to play with it a bit more. So targeting the next release and I'll get 0.13 out of the door by the end of the month.

@waylan
Copy link
Member

waylan commented May 21, 2015

I like it.

@d0ugal d0ugal modified the milestones: 0.14.0, 0.15.0 Jun 4, 2015
@d0ugal d0ugal changed the title Experimentation with theme loading Move builtin themes to external packages other than MkDocs and ReadTheDocs Jun 7, 2015
@d0ugal
Copy link
Member Author

d0ugal commented Jun 27, 2015

❤️ @appveyor's new speed.

d0ugal added a commit that referenced this pull request Jun 27, 2015
Move builtin themes to external packages other than MkDocs and ReadTheDocs
@d0ugal d0ugal merged commit ff05df3 into mkdocs:master Jun 27, 2015
@d0ugal d0ugal deleted the package-themes branch June 27, 2015 10:36
@d0ugal
Copy link
Member Author

d0ugal commented Jun 27, 2015

+697 −12,897. So happy.

@tomchristie
Copy link
Member

+697 −12,897.

✨ 😌 ✨

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

Successfully merging this pull request may close these issues.

None yet

6 participants