Skip to content

Allow to override html_context values #2971

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

Closed
ivankravets opened this issue Jun 26, 2017 · 24 comments
Closed

Allow to override html_context values #2971

ivankravets opened this issue Jun 26, 2017 · 24 comments
Labels
Needed: design decision A core team decision is required

Comments

@ivankravets
Copy link

Details

Expected Result

@platformio documentation is located in separate repository which is "mounted" to core repository.
We override "html_context" with own settings for Github, see conf.py.

Also, see build log and cat docs/conf.py.

So, the "Edit" links still follow to "platformio-core" repository instead of "platformio-docs".

Actual Result

"Edit" links should follow to "platformio-docs" repository.

@agjohnson
Copy link
Contributor

Does this work locally for you? If not, this is an issue with our theme, otherwise, an issue with an override we are applying here.

@agjohnson agjohnson added the Support Support question label Jul 7, 2017
@ivankravets
Copy link
Author

Yes, it works locally.

@ivankravets
Copy link
Author

Do you have any news? Thanks!

We have related report platformio/platformio-docs#19 (comment) by @giogziro95

@akien-mga
Copy link
Contributor

akien-mga commented Mar 6, 2018

The issue is still valid, it works locally with sphinx + sphinx_rtd_theme but not on rtd.org.

Example: https://github.com/godotengine/godot-docs/blob/3.0/conf.py#L69-L76
Resulting link still points to the 3.0 branch. This is quite annoying as our stable 3.0 branch and development master branch are still very similar, and there is no reason in our workflow for PRs to be made against the stable branch (but that's where all contributors are led by the "Edit on GitHub" link).

github_version is set here to remote_version: https://github.com/rtfd/readthedocs.org/blob/d4c21b95e6fcaf69e914aea6b5ed0e0cef1c29d8/readthedocs/doc_builder/backends/sphinx.py#L73

@ivankravets
Copy link
Author

@agjohnson could you fix that? Thanks!

@stsewd
Copy link
Member

stsewd commented Mar 8, 2018

RTD overrides the html_context

https://github.com/rtfd/readthedocs.org/blob/e923c0cdf7547e63b4d2a9ab2b5e69b527bb482b/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl#L76-L133

I think some values can't be allowed to be overridden by the user, but some values like

https://github.com/platformio/platformio-docs/blob/58574c074149656f1cfc5290122251ea6a5344c6/conf.py#L281-L285

make sense to allow the user do it. Probably this could be solved having a black list of settings that the user can't override. Something like

required_context = {
  'using_theme': using_rtd_theme,
  'html_theme': html_theme,
  'MEDIA_URL': "{{ settings.MEDIA_URL }}",
  'PRODUCTION_DOMAIN': "{{ settings.PRODUCTION_DOMAIN }}",
  ...
}
context = {
    'github_user': '{{ github_user }}',
    'github_repo': '{{ github_repo }}',
    'github_version': '{{ github_version }}',
    'display_github': {{ display_github }},
    ...
}
if 'html_context' in globals():
    context.update(html_context)
context.update(required_context)
html_context = context

@humitos
Copy link
Member

humitos commented Mar 9, 2018

We are trying to build a new context for similar issues at #3490 . Would you like to take a look at and give us some feedback?

In fact, I added this note https://github.com/rtfd/readthedocs.org/pull/3490/files#diff-bcc75ef2283eeb5c14031ea7d530d5a8R155 exactly for the same reason that this issue raised here. So, if we want to allow changing this behaviour we are still on time, I guess :)

@akien-mga
Copy link
Contributor

I've had a quick look over the new documentation, but indeed as long as this stands, we can't customize much:

Take into account that the Read the Docs context is injected after your definition of html_context so, it's not possible to override Read the Docs context values.

Wouldn't it be better for the Read the Docs context to be prepended to the user-defined html_context, to let users override it as they wish? If they break their RTD build with a bad custom context, it's their responsibility.

@stsewd
Copy link
Member

stsewd commented Nov 20, 2018

We have a flag that allows projects to override the html_context from rtd (we need to setup this on the db)

https://github.com/rtfd/readthedocs.org/blob/c37b00995c1bbc5ee51d3552ef176546373bb912/readthedocs/doc_builder/templates/doc_builder/conf.py.tmpl#L119-L125

I also think that rtd should allow to override this values by default, not sure how else we can fix this issue.

@stsewd stsewd added the Needed: design decision A core team decision is required label Nov 20, 2018
@ivankravets
Copy link
Author

We use a temporary solution with replacing links via DOM

@stsewd stsewd changed the title Version Control System Integration with Github does not work Allow to override html_context values Nov 27, 2018
@humitos
Copy link
Member

humitos commented Apr 28, 2019

I think this issue is solved by the feature flag DONT_OVERWRITE_SPHINX_CONTEXT (https://docs.readthedocs.io/en/latest/guides/feature-flags.html) and can be closed at this point.

Please, let me know if you want to enable this feature flag in any of your projects and I will do it for your.

If there is anything actionable in this issue, I will close it soon.

@humitos humitos added Needed: more information A reply from issue author is required and removed Needed: design decision A core team decision is required labels Apr 28, 2019
@stsewd
Copy link
Member

stsewd commented Apr 29, 2019

@humitos I'm still not sure why we override by default. Feature flag feels like the default behavior for me

@humitos
Copy link
Member

humitos commented Apr 29, 2019

It's legacy. I suppose they should not avoid other people to override RTD internals here. I'd 👍 on changing this behavior if it does not break projects --which is hard to know.

@no-response
Copy link

no-response bot commented May 12, 2019

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further. Thanks!

@no-response no-response bot closed this as completed May 12, 2019
@akien-mga
Copy link
Contributor

I still think this feature flag should be the default behaviour, or at least something users can enable themselves in their conf.py or RTD dashboard...

Still, in the meantime, I'd be glad if it could be enabled for godot: https://readthedocs.org/projects/godot/

@stsewd
Copy link
Member

stsewd commented May 13, 2019

I'd +1 on changing this behavior if it does not break projects

I think it will only break projects that are already broken locally, I mean building outside rtd.

@humitos
Copy link
Member

humitos commented May 14, 2019

Still, in the meantime, I'd be glad if it could be enabled for godot: https://readthedocs.org/projects/godot/

@akien-mga I've enabled this in godot. Please, let me know if it works as you expected. Otherwise, open a new issue so we can track it there. Thanks!

@ivankravets
Copy link
Author

:) Oh... Why did a bot close this issue? :) Could someone summarize what should be done on our https://docs.platformio.org side?

@no-response no-response bot removed the Needed: more information A reply from issue author is required label May 14, 2019
@no-response no-response bot reopened this May 14, 2019
@stsewd
Copy link
Member

stsewd commented May 14, 2019

@ivankravets it was closed automatically because it didn't have any activity.

If you need to override html_context let us know, and we can activate a flag for your project. We are going to sample some projects before making this the default.

@stsewd stsewd added Needed: design decision A core team decision is required and removed Bug A bug labels May 14, 2019
@ivankravets
Copy link
Author

Aha, got you. Please activate for our project too. Currently, we overwrite "Edit page" with JS hook. Thanks!

@stsewd
Copy link
Member

stsewd commented May 14, 2019

@ivankravets done!

@ivankravets
Copy link
Author

Thanks! It works now!

@stsewd
Copy link
Member

stsewd commented May 14, 2019

We are keeping this issue open till we came with a final decision if enabling the flag by default doesn't break anything :)

@stsewd stsewd reopened this May 14, 2019
@humitos
Copy link
Member

humitos commented May 15, 2019

The original issue is solved. The discussion if enabling it by default or not is going to happen in another place since we need more data to make it. There is no need to keep this issue open here until then.

There are not too many users with this problem and we have a solution (feature flag) for the ones that need it (just need to ask for it). I'm closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

No branches or pull requests

6 participants