-
Notifications
You must be signed in to change notification settings - Fork 212
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
Expand documentation on API media properties #4225
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/4225 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. Changed files 🔄: |
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've included some editorial nits. Take them or leave them, none are critical in any sense, mostly just oriented towards simplifying the text.
Edit: I didn't mean to submit this review, I was just drafting that note into my review comment 🤦 My full review will come shortly.
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 question the use of "Represents" as a necessary preamble to model doc strings. I suspect this is to appease the documentation linter, but these aren't imperative classes, they're declarative data representations. As such, the imperative tone is unnecessarily wordy and even strangely abstract. I'd strongly recommend dropping that conceit from model doc strings and instead stating in simple terms what the model is rather than what it does. "Represents user-submitted reports of audio tracks" becomes "User-submitted reports of audio tracks".
Edit: I did it again! I'm so sorry for repeated pings here 🤦 I'm still in the process of reviewing.
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.
Okay, this is my actual final review. Only the following issues are blockers, the rest are soft suggestions or merely editorial nits:
- We should not duplicate documentation between referent fields and the thing referred. For example, "tags" should not duplicate the documentation on the tag schema and especially should not concern itself with the properties of the tag. The tag's documentation bears this responsibility. For a given work, "tags" should be documented only as "The tags given to a work" (as an example).
- Do not alias
dedent
tod
. It hurts readability. I think the instances it is used, however, would be removed by addressing the previous item. - Drop the imperative tone from model doc strings, preferring instead to document what a model is rather than what it does, in particular because what it does is implicitly "representation", due to the fact that it is a data model. They do not have operative meaning, only representational, despite the fact that individual methods might be operative, in particular,
save
. Those methods of course make sense to have the imperative tone, but on a data model class it's at best strangely abstract and at worst complicates documentation for the sake of a "style" that shouldn't be universally applied.- This will probably require modifying the linting rules. However, I consider it a blocker because the additions in this PR are wide-spread, and addressing this after the fact is additional work we can avoid now. On top of which, it is worse documentation that we would produce if we ignored the linter in this case and used direct terms relevant for the context.
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.
Meant to "request changes", seems like I can't get review submission right at all on this one... sorry!
9996ee3
to
fa067bf
Compare
@sarayourfriend I believe I've addressed most of your feedback, except around the JSON fields help text. This PR is centered around adding documentation that's most helpful for maintainers, which will be rolled up into the API media properties page. This, as far as I can tell, is not the documentation that's surfaced on our OpenAPI documentation, as that information comes from the serializers themselves. I think having different intentions behind each kind of documentation is important - the serializer help text is what gets surfaced to users, and the model help text is what's useful for maintainers in discerning what's actually present in the database. This PR's purview is the latter case. |
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.
LGTM. As mentioned in the thread, I had the wrong downstream context in mind when reviewing these changes, and didn't realise it wouldn't change the OpenAPI schema docs.
I still wish there was a better and clearer way to document the contents of the JSON field, but I guess that's what the JSON field is always going to be anyway: a blob of whatever gets put there, rather than a particular protocol/dataclass/etc.
Looks to me as is now 👍 Apologies for the confusion from my misunderstanding of the downstream changes from this.
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.
LGTM, thanks for filling out all the help texts!
Co-authored-by: sarayourfriend <[email protected]>
Co-authored-by: Dhruv Bhanushali <[email protected]>
b93d52b
to
d11bee1
Compare
This PR has migrations. Please rebase it before merging to ensure that conflicting migrations are not introduced. |
Fixes
Fixes #4024 by @dhruvkb
Description
This PR fills out documentation information for nearly all the API fields. Where possible, I've added more information about what should be contained in certain
JSONField
s.I also added some automatic linking of related model docstrings to the generated documentation output. It seemed prudent to leverage that over repeating the documentation as help text which may go out of date.
A migration has been added for this change, but per
sqlmigrate api 0060
, it's all no-ops:Testing Instructions
Check out the rendered document and make sure all the new additions make sense and are correct!
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin