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

Feature flag to make readthedocs theme default on MkDocs docs #4802

Merged
merged 2 commits into from Oct 24, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Oct 24, 2018

Historically, we were using readthedocs as default theme for MkDocs but in #4556 we decided to change it to get the same behavior when building locally than in Read the Docs.

This commit adds a Feature flag to keep having the old behavior for some particular projects so we can add these project and do not break their documentation (change the theme without asking/reason).

Historically, we were using `readthedocs` as default theme for MkDocs
but in #4556 we decided to
change it to get the same behavior when building locally than in Read
the Docs.

This commit adds a Feature flag to keep having the old behavior for
some particular projects so we can add these project and do not break
their documentation (change the theme without asking/reason).
@humitos humitos requested review from davidfischer, and agjohnson Oct 24, 2018
@humitos humitos self-assigned this Oct 24, 2018
@codecov
Copy link

@codecov codecov bot commented Oct 24, 2018

Codecov Report

Merging #4802 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4802      +/-   ##
==========================================
+ Coverage   76.22%   76.26%   +0.03%     
==========================================
  Files         158      158              
  Lines       10025    10034       +9     
  Branches     1264     1267       +3     
==========================================
+ Hits         7642     7652      +10     
  Misses       2039     2039              
+ Partials      344      343       -1
Impacted Files Coverage Δ
readthedocs/projects/models.py 84.07% <100%> (+0.03%) ⬆️
readthedocs/doc_builder/backends/mkdocs.py 90.09% <100%> (+0.85%) ⬆️
readthedocs/doc_builder/base.py 79.41% <0%> (+1.47%) ⬆️

Loading

if self.project.has_feature(Feature.MKDOCS_THEME_RTD):
if 'theme' not in user_config:
# mkdocs<0.17 syntax
user_config['theme'] = self.DEFAULT_THEME_NAME
Copy link
Contributor

@agjohnson agjohnson Oct 24, 2018

Choose a reason for hiding this comment

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

The thing I think @davidfischer got caught up on here is that this might not work/break when a user pins mkdocs>0.17. Can you confirm the user has a way to self-select out of our feature flag?

Loading


operations = [
migrations.RunPython(forward_add_feature, reverse_add_feature),
]
Copy link
Contributor

@agjohnson agjohnson Oct 24, 2018

Choose a reason for hiding this comment

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

If we're not doing anything like setting a default with the migration, then we don't actually need it for feature flags to work. I guess we'll have to manually add a feature flag on the commercial side?

Loading

Copy link
Member Author

@humitos humitos Oct 24, 2018

Choose a reason for hiding this comment

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

Well, the migration in this case only creates the Feature flag object in the database.

If we don't want that, the migration is useless and I can remove it. As you prefer.

Loading

Copy link
Contributor

@agjohnson agjohnson Oct 24, 2018

Choose a reason for hiding this comment

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

Yeah, this is not required for feature flags. It's only needed if we are setting a time-based default.

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

I think things look correct, but some of the logic might need to be tuned. I'll defer to @davidfischer as he probably has more background into if this will work for our current case and for users that pin mkdocs>0.17.

Loading

stsewd
stsewd approved these changes Oct 24, 2018
# README: historically, the default theme was ``readthedocs`` but in
# https://github.com/rtfd/readthedocs.org/pull/4556 we change it to
# ``mkdocs`` to maintain the same behavior in Read the Docs than
# building locally. Although, we can't apply the this into the Corporate
Copy link
Member

@stsewd stsewd Oct 24, 2018

Choose a reason for hiding this comment

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

the this -> this

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 24, 2018

  • In MkDocs >= 1.0, this will never be a problem as theme is required. Therefore the default will never be used.
  • In MkDocs >= 0.17 but < 1.0, the theme can be specified in mkdocs.yml as a dict although it is optional. If the user specified it, I believe this PR won't do anything.
  • In MkDocs < 0.17, the theme was always just a string although it was optional. This should handle that just fine.

Loading

@humitos
Copy link
Member Author

@humitos humitos commented Oct 24, 2018

In MkDocs >= 1.0, this will never be a problem as theme is required. Therefore the default will never be used.

Why the default will never be used? Because it's mandatory to be a dict like theme.name? If they were using mkdocs>=1.0 they need to be setting the theme manually otherwise, it shouldn't be working right now.

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 24, 2018

Why the default will never be used? Because it's mandatory to be a dict like theme.name? If they were using mkdocs>=1.0 they need to be setting the theme manually otherwise, it shouldn't be working right now.

This is all correct. In 1.0+, theme.name is mandatory. Nobody is using 1.0+ unless they specify it in their requirements file.

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

Looks good. I ran a quick test with the test project with no theme specified.

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Oct 24, 2018

Great, sounds like this can be merged and hotfixed on our release branch.

Loading

@agjohnson agjohnson merged commit 188130a into master Oct 24, 2018
3 checks passed
Loading
@stsewd stsewd deleted the humitos/featureflag/mkdocs-theme branch Oct 24, 2018
@agjohnson agjohnson restored the humitos/featureflag/mkdocs-theme branch Oct 24, 2018
@humitos humitos deleted the humitos/featureflag/mkdocs-theme branch Oct 24, 2018
@humitos humitos restored the humitos/featureflag/mkdocs-theme branch Oct 24, 2018
@stsewd stsewd deleted the humitos/featureflag/mkdocs-theme branch Oct 30, 2018
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