Added GitHub Wiki pages on adding testing/documentation to Sphinx Docs #1137
Conversation
Updated contributing.rst and related files Fixed some grammar, added pre-commit section
jsonvillanueva
added
documentation
jsonvillanueva
changed the title
jsonvillanueva
added
the
pr:easy review
Co-authored-by: Naveen M K <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
Co-authored-by: Naveen M K <[email protected]>
github-actions
bot
removed
the
pr:dependent
There was a problem hiding this comment.
Thanks for moving the wiki pages to our documentation!
While reading, I noticed that in particular the section on adding typings doesn't really represent our current recommended best practice (type hints in the code, as opposed to specifying the types in the docstring) any longer. However, I think these issues should be addressed in a new PR.
I've added some minor suggestions; if you agree with them, then please apply them -- and otherwise, this can (IMO) be merged.
| the class docstring**, *rather than under* ``__init__``. Note that this | ||
| can be omitted if the parameters and the attributes are the exact same | ||
| (i.e., dataclass) - priority should be given to the ``Attributes`` | ||
| section, in this case, which must **always be present**, unless the | ||
| class specifies no attributes at all. (See more on Parameters in number | ||
| 2 of this list.) |
There was a problem hiding this comment.
We don't really follow this particular point regarding the Attributes-section. I'd encourage users to add it, but I don't think it would be fair to enforce it.
There was a problem hiding this comment.
How would you recommend updating this? Just removal of sentences involving Attributes?
| name : :class:`str` | ||
| The class's name. | ||
| id : :class:`int` | ||
| The class's id. | ||
| mobj : Optional[:class:`~.Mobject`], optional | ||
| The mobject linked to this instance. Defaults to `Mobject()` \ | ||
| (is set to that if `None` is specified). |
There was a problem hiding this comment.
Should we explicitly mention that instead of specifying types in the docstring, using type hints would be preferred?
There was a problem hiding this comment.
I agree that we should urge developers to enforce #52, or try to do type hints. I'll do a draft of this in a future commit to make the language more direct on our preference.
| Type specifications | ||
| ~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
The following section should also rather suggest type hints over adding the types in the docstrings.
Co-authored-by: Benjamin Hackl <[email protected]>
|
On second thought, it would be better to provide further clarification/updates to our documentation guidelines in a separate PR. The main takeaway from this PR is to urge would-be contributors to read our the Whenever that future PR is made (might not be by me), it'll still be relevant between releases when made. I've opened an issue to follow-up this PR |
jsonvillanueva
changed the title
2 participants
Motivation
The GitHub Wiki pages are often overlooked when contributing to Manim. By providing these pages under our sphinx documentation, contributors are more likely to properly install Manim for development and more easily find appropriate sections for their specific contributions.
Acknowledgements
Reviewer Checklist