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

Documentation for RTD context sent to the Sphinx theme #3490

Merged
merged 17 commits into from Mar 23, 2018

Conversation

Projects
None yet
4 participants
@humitos
Member

humitos commented Jan 9, 2018

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

@humitos humitos requested a review from ericholscher Jan 9, 2018

@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.

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

This comment has been minimized.

@agjohnson

agjohnson Jan 9, 2018

Contributor

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

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

This comment has been minimized.

@agjohnson

agjohnson Jan 9, 2018

Contributor

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

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

This comment has been minimized.

@agjohnson

agjohnson Jan 9, 2018

Contributor

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jan 9, 2018

Contributor

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

This comment has been minimized.

Contributor

agjohnson commented Jan 9, 2018

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

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

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.

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

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.

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

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

This is missing subprojects

This comment has been minimized.

@humitos

humitos Jan 15, 2018

Member

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?

This comment has been minimized.

@ericholscher

ericholscher Jan 15, 2018

Member

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.

This comment has been minimized.

@humitos

humitos Jan 16, 2018

Member

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.

This comment has been minimized.

@humitos

humitos Jan 16, 2018

Member

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.

.. 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.

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

Grammar issue here at:
to avoid versions to be inconsistent

This comment has been minimized.

@humitos

humitos Jan 15, 2018

Member

I don't know how to fix it :)

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

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

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

supplied -> supplies

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

This comment has been minimized.

@agjohnson

agjohnson Jan 15, 2018

Contributor

is inject -> is injected

@humitos

This comment has been minimized.

Member

humitos commented Jan 15, 2018

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

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

This comment has been minimized.

@ericholscher

ericholscher Jan 15, 2018

Member

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.

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

This comment has been minimized.

@ericholscher

ericholscher Jan 15, 2018

Member

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}/`

This comment has been minimized.

@humitos

humitos Jan 16, 2018

Member

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)

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

This comment has been minimized.

@ericholscher

ericholscher Jan 15, 2018

Member

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.

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

This comment has been minimized.

@ericholscher

ericholscher Jan 15, 2018

Member

Should probably be a warning.

@humitos

This comment has been minimized.

Member

humitos commented Jan 16, 2018

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.

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`_.

This comment has been minimized.

@Blendify

Blendify Feb 5, 2018

Contributor

"on" --> "in the"

@Blendify Blendify referenced this pull request Feb 6, 2018

Closed

0.3.0: Remaining todos #556

4 of 8 tasks complete
@Blendify

This comment has been minimized.

Contributor

Blendify commented Feb 8, 2018

@agjohnson does everything look good now?

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

This comment has been minimized.

@Blendify

Blendify Feb 9, 2018

Contributor

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

This comment has been minimized.

@humitos

humitos Feb 13, 2018

Member

This code will be injected (when this docs get coded) at this point:

... 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? :)

This comment has been minimized.

@Blendify

Blendify Feb 17, 2018

Contributor

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

This comment has been minimized.

@humitos

humitos Feb 20, 2018

Member

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

@Blendify Blendify requested a review from ericholscher Feb 17, 2018

@Blendify

This comment has been minimized.

Contributor

Blendify commented Feb 20, 2018

Yep looks good

@ericholscher

This comment has been minimized.

Member

ericholscher commented Mar 7, 2018

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

This comment has been minimized.

Contributor

agjohnson commented Mar 9, 2018

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

This comment has been minimized.

Member

ericholscher commented Mar 12, 2018

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

This comment has been minimized.

Member

humitos commented Mar 12, 2018

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

This comment has been minimized.

Member

ericholscher commented Mar 14, 2018

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

This comment has been minimized.

Contributor

agjohnson commented Mar 15, 2018

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

@agjohnson agjohnson merged commit c5d102a into master Mar 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@agjohnson agjohnson deleted the humitos/docs/template-context branch Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment