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

Do not overwrite sphinx context variables feature #4349

Merged
merged 2 commits into from Jul 11, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Jul 9, 2018

This adds a feature flag (default off) that changes how html_context variables are clobbered in conf.py. By default, we clobber whatever the user may have in html_context if it matches variable names set by Read the Docs. This changes the setting to respect what a user may have and only overwrite if the user didn't set anything.

@davidfischer davidfischer requested a review from Jul 9, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Nice! Looks good to me 👍

I think if we did a sample of projects, this will be a good test that helps us answer how backwards compatible we need to be when implementing a versioned context_data.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Jul 10, 2018

I think if we did a sample of projects, this will be a good test that helps us answer how backwards compatible we need to be when implementing a versioned context_data.

Agreed

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 10, 2018

I think if we did a sample of projects, this will be a good test that helps us answer how backwards compatible we need to be when implementing a versioned context_data.

It's going to be almost impossible to tell if these builds fail or have unexpected behavior because of the change though. If we logged or collected metrics on the number of times we hit a user-specified key that would probably be the most valuable.

@humitos
Copy link
Member

@humitos humitos commented Jul 10, 2018

If we logged or collected metrics on the number of times we hit a user-specified key that would probably be the most valuable

I like it. This metric could help us to understand the context better than a failed build. Do we have a table for saving this kind of things already?

Copy link
Member

@humitos humitos left a comment

Good and simple enough!

I want to mention that I would like to do the same for the readthedocs.v1 context by default (the new design) and remove the warning from the bottom of this page: https://docs.readthedocs.io/en/latest/design/theme-context.html#customizing-the-context and allow users to override only one key and not the whole readthedocs.v1 object.

Of course, this is something to be done in a completely different PR.

@davidism
Copy link

@davidism davidism commented Jul 10, 2018

There's a typo, SPINX -> SPHINX.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Jul 11, 2018

The typo is fixed.

@davidfischer davidfischer merged commit 9c8fa66 into master Jul 11, 2018
1 check passed
@davidfischer davidfischer deleted the davidfischer/dont-overwrite-sphinx-context branch Jul 11, 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

5 participants