Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Design doc: Embed APIv3 #8222
Design doc: Embed APIv3 #8222
Changes from 16 commits
9d4ebe5
fe633ec
66d839e
06c8a97
7448158
1d3c097
1d43423
81edc9c
89a5795
4efd3ec
adebe39
e307563
f783339
efe5abc
145b6e8
5063bbc
84c7ce0
3b320e8
b34fce0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this useful? Seems like a lot of content to return -- is the use case here for small pages or browsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it because it's how it works currently. I don't have a strong opinion and I'm happy to remove it if there is no well-defined use case yet. In fact, I didn't add this originally but then I came back and added this paragraph because I realized that I was removing a feature that we currently have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I'm not sure. I use this functionality a lot when tested (eg. I just throw a URL at the API and it returns the whole page content) -- but I'm not sure if this is generally useful. It definitely makes the API easier to browse and understand, so I think that's valuable. A lot of users will probably do this instead of any
metadata
endpoint to get the page sections.I think this functionality probably makes sense, but we should be very clear about what it does (no
etc
:D)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we are already using the feature as is right now. However, I opened a PR in sphinx-hoverxref to support
:doc:
that would require this: readthedocs/sphinx-hoverxref#68He he, I left the "etc" here because I didn't do the research required to grab the most used ones. The two that I mentioned here are HTML5 and Sphinx default's one, but it may be others. This will be clear in the documentation of the endpoint itself without magic/guessing involved, tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this feature combined with
?nparagraphs=
makes more sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a better solution than doing it for all requests, and gives us a way to deprecate things 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are specifying deprecation in this document, then the use case for embedding Sphinx refs should also be mentioned here. Deprecation should depend on surfacing an API to expose documentation refs. We need to have a discussion about this end point and how it is implemented/etc next, but that can be separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a good question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to switch the version and API name so we have the version after its name? That way we can release a new version of a particular API without touching the others.
/api/search/v3/
/api/footer/v3/
/api/resources/v2/
and/api/resources/v3/
/api/embed/v3/
cc @stsewd @ericholscher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That naming feels weird to me, I think we just need to document which endpoints are part of what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stsewd can you expand how that would be and give some examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already done in part for search for example https://docs.readthedocs.io/en/stable/server-side-search.html#api we just need to do the same with the embed api and footer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stsewd I don't follow you. That search endpoint is under
/api/v2/
--so we are not differentiating the "Feature APIs" from the "Resources API" there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to make this distinction in our docs, not in the URLs