Documentation for RTD context sent to the Sphinx theme #3490

humitos · 2018-01-09T01:01:04Z

This is an initial proposal for #3482 where I deleted the vcs.rst page since looks too obsolate for me and I added a new one theme-context.rst that explains all the information send to the Sphinx template but also propose a refactor of the data sent under a namespace. Besides, I removed a couple of unnecessary variables and reorder others.

Closes #3482

agjohnson

Raising some questions on the spec to start. I haven't looked at the copy so I'll mark this as changes required.

The overlap here that I discussed is that its currently difficult to take the information we're passing in to the theme and make calls against the API, as pks are missing from all of the lists of objects. This enables a landing page style project where templates can call our API and get subproject version and build information.

agjohnson · 2018-01-09T01:30:49Z

docs/theme-context.rst

+
+    'readthedocs': {
+        'version': {
+            'name': str,


So on the version data, any reason not to just use a Version object serialized through DRF?

agjohnson · 2018-01-09T01:31:29Z

docs/theme-context.rst

+                'epub': str
+            }
+        },
+        'project': {


Similar to above, I think we could just use a serialized Project instance here.

agjohnson · 2018-01-09T01:32:28Z

docs/theme-context.rst

+            'single_version': bool,
+            'versions': [
+                {
+                    'slug': str,


More specifically, it is really helpful to be able to hit the API to fetch additional version/subproject information. If putting a fully serialized version object here doesn't make sense, we at least could use a version pk in this dict.

agjohnson · 2018-01-09T01:33:18Z

docs/theme-context.rst

+            ],
+            'subprojects': [
+                {
+                    'slug': str,


Similar to above regarding the list of versions, having more subproject fields here would be helpful for making a landing page, for example. This should either be a serialized Project, or at very least should include a pk.

agjohnson · 2018-01-09T01:39:35Z

If it makes sense, structuring this all as a DRF serializer might make sense, because we will eventually replace the footer html API endpoint with an endpoint that just returns the above context data. The client side JS will build up a navigation menu, not just copy HTML in.

agjohnson

I think the new spec looks good! There are some missing pieces we need to ensure get added however. I noted some grammar problems, but feel free to bring any of us in to clean that up. I noted so we can remember to clean that up before merge.

agjohnson · 2018-01-15T08:47:15Z

docs/theme-context.rst

+
+.. _html_context Sphinx setting: http://www.sphinx-doc.org/en/stable/config.html#confval-html_context
+
+Context injected by default


"by default" implies there is a non-default injection, which isn't user configurable. I think this can be dropped from the heading

agjohnson · 2018-01-15T08:49:46Z

docs/theme-context.rst

+Before calling `sphinx-build` to render your docs, Read the Docs injects some
+extra context in the templates by using the `html_context Sphinx setting`_ in
+the ``conf.py`` file. This extra context allows Read the Docs to add some
+features like "Edit on GitHub" and use a user custom Analytics code, among others.


Instead of describing the specific features, it's probably easiest to just say something along the lines of:

This extra context is used by the Read the Docs Sphinx theme to add additional features to the built documentation.

agjohnson · 2018-01-15T08:51:17Z

docs/theme-context.rst

+            'MEDIA_URL': str, 
+            'PRODUCTION_DOMAIN': str,
+            'READTHEDOCS': True
+        }


This is missing subprojects

Mjm, I finally removed the subprojects because I supposed this was dynamic data that should be retrieved by JS. I wasn't really sure what to do here.

Isn't "dangerous" to make it static under this context? Won't this give us some problems in the future?

I'm still not convinced on this argument about injecting dynamic data. It still seems like something that users could want for various reasons in their doc context, without having to render it via JS/API. "It might go out of date when a new version is built" doesn't seem like a compelling enough reason to not include the live data when the build happens. What if they just want to include a subset of the data that they know will always be valid, for example? Just because our user case requires fresh data, doesn't meal all use cases do.

We should warn users of the possible downsides of using this data, but not passing it in feels wrong to me.

Good!

I re-structure the notes to be warnings and communicate the user some of the problems of using this static data and what would be the solution (using the API via JS). I wrote the idea, probably the English could be improved.

I also added the subprojects to the project object; I'm not sure if I have to add the versions also under this object or not.

agjohnson · 2018-01-15T08:52:45Z

docs/theme-context.rst

+.. note::
+
+   By design, Read the Docs passes only static information to `sphinx-build`
+   to avoid versions to be inconsistent in case the project is updated after the version is built.


Grammar issue here at:
to avoid versions to be inconsistent

I don't know how to fix it :)

"keep" instead of "to be"? :/

agjohnson · 2018-01-15T08:52:59Z

docs/theme-context.rst

+
+   By design, Read the Docs passes only static information to `sphinx-build`
+   to avoid versions to be inconsistent in case the project is updated after the version is built.
+   In case you need more information than the context supplied here, you will


supplied -> supplies

agjohnson · 2018-01-15T08:54:14Z

docs/theme-context.rst

+
+.. note::
+
+   Take into account that the Read the Docs context is inject after your definition of ``html_context`` so,


is inject -> is injected

humitos · 2018-01-15T14:54:42Z

I worked on all the suggestions made by @agjohnson then only piece missing is the subprojects value. I left a question there.

ericholscher · 2018-01-15T15:55:03Z

docs/theme-context.rst

+Before calling `sphinx-build` to render your docs, Read the Docs injects some
+extra context in the templates by using the `html_context Sphinx setting`_ in
+the ``conf.py`` file. This extra context is used by the Read the Docs Sphinx Theme
+to add additional features to the built documentation


Well, we use it, but the goal of this doc is to also make it usable by the user. I think we should communicate to the user why they should care about this doc, not why we use it. Do they really care why we use it? If so, it could easily be a section lower in the document explaining our usage, but not part of the introduction to the feature.

ericholscher · 2018-01-15T15:56:24Z

docs/theme-context.rst

+                'htmlzip': str,
+                'epub': str
+            },
+            'resource_uri': '/api/v2/version/{pk}/'


is resource_uri a standard term we use? I don't see it in our existing API v2 return values. Also, I think we should put a fully qualified URL (eg. https://readthedocs.org/api/v2/version/{pk}/`

I took that name from the APIv1 http://docs.readthedocs.io/en/latest/api.html?highlight=resource_uri#get--api-v1-build-id-

But the standard way seems to be to add a links object like:

"links": [ { "href": "https://readthedocs.org/api/v2/version/{pk}/", "rel": "self" } ]

(I think we shouldn't make it a list, just a dict --but that is not standard)

https://restfulapi.net/hateoas/

https://spring.io/understanding/HATEOAS

ericholscher · 2018-01-15T16:00:15Z

docs/theme-context.rst

+            'MEDIA_URL': str, 
+            'PRODUCTION_DOMAIN': str,
+            'READTHEDOCS': True
+        }


I'm still not convinced on this argument about injecting dynamic data. It still seems like something that users could want for various reasons in their doc context, without having to render it via JS/API. "It might go out of date when a new version is built" doesn't seem like a compelling enough reason to not include the live data when the build happens. What if they just want to include a subset of the data that they know will always be valid, for example? Just because our user case requires fresh data, doesn't meal all use cases do.

We should warn users of the possible downsides of using this data, but not passing it in feels wrong to me.

ericholscher · 2018-01-15T16:02:02Z

docs/theme-context.rst

+    <p>This documentation was written by {{ author }} on {{ date }}.</p>
+
+
+.. note::


Should probably be a warning.

humitos · 2018-01-16T15:48:57Z

OK, I'm marking this one ready for review since I'd like you to take a new look at it and keep discussing. Thanks!

Blendify

LGTM, one small thing.

Blendify · 2018-02-05T23:00:43Z

docs/theme-context.rst

+
+   In this example, we are using ``pagename`` which is a Sphinx variable
+   representing the name of the page you are on. More information about Sphinx
+   variables can be found on `Sphinx documentation`_.


"on" --> "in the"

Blendify · 2018-02-08T14:21:30Z

@agjohnson does everything look good now?

Blendify · 2018-02-09T15:52:06Z

docs/theme-context.rst

+Note that this dictionary is injected under the main key `readthedocs`:
+
+
+.. code:: python


Add a comment about where this info came from / how to update it

This code will be injected (when this docs get coded) at this point: https://github.com/rtfd/readthedocs.org/blob/0c547f47fb9dffbeb17e4e9ccf205a10caf31189/readthedocs/doc_builder/backends/sphinx.py#L65

... and there is no way to "update" RTD values since they are injected at the bottom of the conf.py after the values from the user.

So, do you think we should add that line of code to the docs? or you were thinking in another thing and I didn't get it? :)

I was saying to add code comment for in the future we know where to generate the dictionary from.

If I understood correctly, I already done this. Check my latest commit :)

Blendify · 2018-02-20T17:17:21Z

Yep looks good

ericholscher · 2018-03-07T08:41:48Z

I'm +1 on this, and think it would be a good thing to get moving, now that we have a spec :) The next steps are actually implementing the API described in the PR.

agjohnson · 2018-03-09T23:36:04Z

Changes look good! There are a few more points to raise with the proposed feature decided on:

The above changes actual documentation without changing a feature, perhaps this should live in a /proposal path or something for now?
We don't discuss how namespaces will live alongside each other, or what happened to the old config format, or what that spec even looked like. I'm not sure we need to document a spec that we never bothered to document, but we did delete useful documentation for a spec that we are still using. Restoring the docs that were deleted and moving this to a proposal path probably does enough for now, but we might want to mention that there are also settings in the main namespace from a legacy config.

ericholscher · 2018-03-12T00:54:28Z

I'd say we can move this into a docs/design/ design doc directory, and have it for future reference around the design of this API.

My thought was that this would be a new fancy addition that lives alongside the old API, which we'll keep around for a while. It isn't documented, so in theory we can remove it, but I think a lot of people depend on it, so I'd want to have numbers around usage and a long deprecation before we dropped it fully.

humitos · 2018-03-12T14:39:38Z

docs/proposal or docs/design are fine to me. I think proposal is more explicit regarding "this has not built yet".

We don't discuss how namespaces will live alongside each other, or what happened to the old config format, or what that spec even looked like.

We chatted about this some time ago and we say that we will mark the old one as deprecated and support both at the same time for a while. Then, remove the old one if that it's possible :/

I'm not sure we need to document a spec that we never bothered to document, but we did delete useful documentation for a spec that we are still using. Restoring the docs that were deleted and moving this to a proposal path probably does enough for now

If we don't remove what I've removed, we are documenting a chunk of the code that we want to remove :) --how to inject VCS in your template. Anyway, I could restore it and add a deprecation note pointing to what's planned to implement. What do you think?

but we might want to mention that there are also settings in the main namespace from a legacy config

Which one are those? We may probably want to remove them from the new specs if they are just for legacy compatibility.

so I'd want to have numbers around usage and a long deprecation before we dropped it fully

How do we get these numbers?

Also, also... In this issue was raised the ability to override our own configs, #2971, do we want to support this and rely on the user's configurations?

ericholscher · 2018-03-14T00:48:36Z

I think docs/design is best, since it's a design doc, and it will hopefully be implemented :)

How do we get these numbers?

I'm not sure if we can. It's another ticket if we want to discuss it.

Also, also... In this issue was raised the ability to override our own configs, #2971, do we want to support this and rely on the user's configurations?

We should discuss that there. I think namespacing it in readthedocs makes it so we shouldn't support letting users override it.

agjohnson · 2018-03-15T23:20:55Z

If we don't remove what I've removed, we are documenting a chunk of the code that we want to remove :) --how to inject VCS in your template. Anyway, I could restore it and add a deprecation note pointing to what's planned to implement. What do you think?

For now, that is my point I suppose. We aren't yet implementing any of these changes, and nothing should be deprecated yet (we have nothing in code to replace this context with yet). So until this happens, I'm suggesting our live docs don't reflect changes we want, it should reflect the reality of our production instance.

Which one are those? We may probably want to remove them from the new specs if they are just for legacy compatibility.

There aren't any settings that we're phasing out, I meant that as we move to a new schema for the context data we should probably mention that there is an old schema format, ie:
https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/doc_builder/backends/sphinx.py#L98-L135

I'm not sure we need to document things in depth, but we are a documentation company :) -- we should make not of these things that others are relying on. We won't be able to get rid of this context, or really deprecate it. We can't stop projects from relying on versions of our theme that rely on this context, so we're stuck with producing this context. We should treat this data as first class, just like the next generation of the schema.

cc @humitos

* build_date * HATEOAS for resources

Examples: * http://readthedocs.org/api/v2/project/{id}/ * http://readthedocs.org/api/v2/version/{id}/

humitos requested a review from ericholscher January 9, 2018 01:01

agjohnson requested changes Jan 9, 2018

View reviewed changes

agjohnson added Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review labels Jan 9, 2018

humitos mentioned this pull request Jan 10, 2018

gitlab theme integration not working #3500

Closed

agjohnson requested changes Jan 15, 2018

View reviewed changes

ericholscher reviewed Jan 15, 2018

View reviewed changes

humitos added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Jan 16, 2018

Blendify approved these changes Feb 5, 2018

View reviewed changes

Blendify mentioned this pull request Feb 6, 2018

0.3.0: Remaining todos readthedocs/sphinx_rtd_theme#556

Closed

8 tasks

Blendify reviewed Feb 9, 2018

View reviewed changes

Blendify requested a review from ericholscher February 17, 2018 15:51

humitos mentioned this pull request Mar 9, 2018

Allow to override html_context values #2971

Closed

agjohnson approved these changes Mar 9, 2018

View reviewed changes

humitos added 4 commits March 22, 2018 14:03

Documentation for RTD context sent to the Sphinx theme

c5041a4

Fix index

c77ad57

Added version downloads

b7426bf

Add pk and resource_uri to be able to retrieve fresh data

2bdc191

humitos and others added 13 commits March 22, 2018 14:03

Typos and grammar fixes

17f1055

Improve context

6b82f0e

* build_date * HATEOAS for resources

Add v1 namespace and better explanation about static/dynamic data

540acd9

Update context to follow APIv2 reponses

1a61cc8

Examples: * http://readthedocs.org/api/v2/project/{id}/ * http://readthedocs.org/api/v2/version/{id}/

Add subprojects inside project

d378093

SVN as an option

c60671f

Docs: Correct small typo

d1aaede

Add comment saying where this context comes from

bf3f75e

Re-add VCS documentation file

d138e33

Move proposal design under docs/design/ directory

fb86162

Cleanup extra spaces

b051758

Add note about the new design for the theme context

55a83b6

Update the index

4f911f4

humitos force-pushed the humitos/docs/template-context branch from 1fa3325 to 4f911f4 Compare March 22, 2018 19:04

agjohnson approved these changes Mar 23, 2018

View reviewed changes

agjohnson merged commit c5d102a into master Mar 23, 2018

agjohnson deleted the humitos/docs/template-context branch March 23, 2018 17:47

humitos mentioned this pull request Apr 2, 2018

interface for adding extra template context #3483

Closed

davidfischer mentioned this pull request Jul 6, 2018

allow specifying extras instead of requirements.txt #3484

Closed

stsewd mentioned this pull request Nov 27, 2018

Add ability to override computed template context variables #743

Closed


		.. _html_context Sphinx setting: http://www.sphinx-doc.org/en/stable/config.html#confval-html_context

		Context injected by default


		.. note::

		Take into account that the Read the Docs context is inject after your definition of ``html_context`` so,

		<p>This documentation was written by {{ author }} on {{ date }}.</p>


		.. note::

		Note that this dictionary is injected under the main key `readthedocs`:


		.. code:: python

Documentation for RTD context sent to the Sphinx theme #3490

Documentation for RTD context sent to the Sphinx theme #3490

Conversation

humitos commented Jan 9, 2018

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson commented Jan 9, 2018

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Jan 16, 2018

Blendify left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blendify commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blendify commented Feb 20, 2018

ericholscher commented Mar 7, 2018

agjohnson commented Mar 9, 2018

ericholscher commented Mar 12, 2018 • edited Loading

humitos commented Mar 12, 2018

ericholscher commented Mar 14, 2018

agjohnson commented Mar 15, 2018

ericholscher commented Mar 12, 2018 •

edited

Loading