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

Add support for GitLab repositories #1435

Merged
merged 2 commits into from Mar 15, 2018

Conversation

Projects
None yet
2 participants
@tribut
Contributor

tribut commented Mar 14, 2018

This adds support for GitLab repositories in the same way that already exists for GitHub and Bitbucket.

Note that the icons in the themes will not show up until FontAwesome is updated to at least version 4.6 and/or an up-to-date version of the upstream readthedocs css is imported.

This commit also fixes a slight documentation error regarding the default value of repo_name and removes some tautological tests.

@tribut tribut referenced this pull request Mar 14, 2018

Merged

Add GitLab icon #42

@waylan

Thanks. Other than the one minor nit-pick, this looks good.

**default**: `edit/master/docs/` or `src/default/docs/` for GitHub or Bitbucket
repos, respectively, if `repo_url` matches those domains, otherwise `null`
**default**: `edit/master/docs/` or `src/default/docs/` GitHub, GitLab or
Bitbucket repos, respectively, if `repo_url` matches those domains,

This comment has been minimized.

@waylan

waylan Mar 15, 2018

Member

Missing "for" before "GitHub". Although, "respectively" doesn't really work as there are two items in the first list and three in the second. Sure, users in the know will understand, but we should not assume users will know that GitHub and Gitlab share the same URL scheme. How about:

**default**: `edit/master/docs/` for GitHub and GitLab repos or `src/default/docs/` for a Bitbucket repo...
@waylan

This comment has been minimized.

Member

waylan commented Mar 15, 2018

In view of #1378 I'm wondering if we should even bother with this. Yes, this gives GitLab equal footing with GitHub, which is not a bad thing. However, every time a new service comes along, I don't want to have to add support for it like this. That would be a maintenance nightmare. Perhaps if we take the approach proposed in #1378, then we might be able to avoid the need for this.

What I would really like to do is to rip all of the repo specific stuff out and implement it as a plugin. That gives us the freedom to add all sorts of configuration without adding to the main config. And as new services come along, any third party could create a plugin which worked with that services' specific needs. My hesitation involves how to deal with themes. Plugins can't inject template code into an existing theme unless they override/replace an entire file. And getting a plugin to provide template code that works across all themes, including third party themes, would be nigh impossible. I'm thinking perhaps the solution would be to have the plugin provide a documented list of template variables and templates would need to provide the logic to display those variables (this is more-or-less the approach taken by the search plugin). For example, one of the variables could be the "name" of the icon to display. What names would be valid would depend on the theme you use. For example, a theme which uses FontAwesome might support different names than a theme that used some other mechanism for displaying icons. Consider the following template snippet which would work in the readthedocs theme:

class="icon icon-{{ icon_name }}"

Of course, github, bitbucket, and gitlab would all be valid names there. And we wouldn't need the many conditionals we have now for each specific service.

Thoughts?

Add support for GitLab repositories
Note that the icons in the themes will not show up until FontAwesome is
updated to at least version 4.6 and/or an up-to-date version of the
upstream readthedocs css is imported.

This commit also fixes a slight documentation error regarding the
default value of `repo_name` and removes some tautological tests.

@tribut tribut force-pushed the tribut:gitlab-repos branch from 87202d4 to 062bc08 Mar 15, 2018

@tribut

This comment has been minimized.

Contributor

tribut commented Mar 15, 2018

Hey - I've made the change you suggested. My wording was indeed a bit wonky.

As far as the larger issue is concerned it is certainly true that maintaining the list of potential services here and replicated in all the themes seems unsustainable. I've only opened this PR because I noticed the icon was missing in cinder and that bugged me. Adding a variable for the service/icon name seems reasonable, and maybe a service name is enough (currently the icon name would only be service_name | lower and that already assumes everyone is using FontAwesome).

However, unless you are planning to implement this soon, I'd still suggest merging this PR. Ripping it out again later does no harm.

@tribut

This comment has been minimized.

Contributor

tribut commented Mar 15, 2018

One more thing I just realized: Currently I can set repo_name: GitLab and the icon will be picked up. This works, even if I am using a privately hosted GitLab instance. It would be great if this flexibility would be preserved in a future, plugin-based approach.

@waylan

waylan approved these changes Mar 15, 2018

@waylan

This comment has been minimized.

Member

waylan commented Mar 15, 2018

As this is a fully working implementation for GitLab we'll merge this now. That way, if/when we address the other issues, we'll be sure to include GitLab. Otherwise it may get forgotten about.

@tribut thanks again for doing the work on this.

@waylan waylan merged commit 71ebf35 into mkdocs:master Mar 15, 2018

3 checks passed

codecov/project 94.08% (target 90%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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