-
Notifications
You must be signed in to change notification settings - Fork 215
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
Add implementation plan for documenting the API media properties #3965
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/3965 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. New files ➕: Changed files 🔄:
|
587a06b
to
f35fb26
Compare
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.
@dhruvkb This is looking like a solid document! I don't anticipate more questions or comments than the ones I left here.
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Show resolved
Hide resolved
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
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.
Thanks for updating the details, and writing this comprehensive proposal! Everything looks good to me, so shortcuting the rounds, I approve it 👍
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Krystle Salazar <[email protected]>
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.
It's great that Django has all of the capabilities to allow for documenting the properties! Love the plan and the linked PR with documentation.
I would love to see more detail on the Future improvements
point on jsonb
fields, especially after the incident caused by #3930. Do you think this should be done in a separate project, or as a separate part of this project? Would we add separate serializers to allow us to use introspection, or is there another way available to auto-generate documentation?
I wouldn't consider the "Fully document media properties" project completed without documenting the JSONFields because they are the ones that can cause problems in the future.
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
We will document this information in the docstring of the model classes from | ||
where it can be accessed using `inspect.getdoc`. To ensure that these notes are | ||
co-located with the fields' code, we will parse docstrings of all ancestors and | ||
mixins of a model class. |
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.
Will this work with help_text
that is located in a separate file and is added inside __init__
, as the source
ones?
openverse/api/api/serializers/media_serializers.py
Lines 310 to 311 in 12d7162
self.fields["source"].help_text = SOURCE_HELP_TEXT.format(**variables) | |
self.fields["excluded_source"].help_text = EXCLUDED_SOURCE_HELP_TEXT.format( |
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.
These help texts are taken from the model and not from the serializer. That's because for a specific model a number of serializers may be defined that can have different help_text
s as per the requirements of the endpoint.
So for source, we show this
openverse/api/api/models/media.py
Lines 56 to 58 in 12d7162
help_text="The source of the data, meaning a particular dataset. " | |
"Source and provider can be different. Eg: the Google Open " | |
"Images dataset is source=openimages, but provider=flickr.", |
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.
You can visit https://docs.openverse.org/_preview/3966/meta/media_properties/api.html to see the the generated docs output.
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
I totally agree with this sentiment and I tried earnestly to include them. But the documentation of I have documented these issues in f9dd8c9 and noted some steps we can take to enforce validity in our JSON data, which can then be used to generate documentation. |
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @obulat Excluding weekend1 days, this PR was ready for review 6 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @dhruvkb, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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.
Thank you for adding information on the json fields and possible solutions.
The plan looks good, and I'm short-circuiting this discussion to approve it.
.../document_all_media_properties/20240313-implementation_plan_document_media_properties_api.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Olga Bulat <[email protected]>
Fixes
Fixes #1996 by @obulat
Description
This PR adds the implementation plan for documenting the media properties in the API. The IP uses introspection as the primary approach along with a documentation generation plan similar to that for the catalog.
Reviewers
I have requested reviews from @obulat and @krysal. The reason for the selection is because @obulat was involved in the IP and implementation for the corresponding project at the catalog layer and because IIRC @krysal has done prior work in the area of media properties documentation.
Testing Instructions
Please read the IP and comment your opinions. You can also see the implementation of this plan at #3966.
Decision-making process
This discussion is following the Openverse decision-making process. Information about this process can be found on the Openverse documentation site. Requested reviewers or participants will be following this process. If you are being asked to give input on a specific detail, you do not need to familiarise yourself with the process and follow it.
Current round
This discussion is currently in the Clarification round. The deadline for review of this round is 5 April 2024.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin