Skip to content
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

Enhance documentation for contributing new checks #546

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

bendichter
Copy link
Contributor

  • Added a new guide on contributing checks to NWBInspector in contributing_checks.rst.
  • Updated developer_guide.rst to reference the new contributing checks guide.
  • Modified .gitignore to exclude additional build artifacts.

This update aims to streamline the contribution process and improve clarity for new contributors.

bendichter and others added 6 commits December 27, 2024 11:20
- Added a new guide on contributing checks to NWBInspector in `contributing_checks.rst`.
- Updated `developer_guide.rst` to reference the new contributing checks guide.
- Modified `.gitignore` to exclude additional build artifacts.

This update aims to streamline the contribution process and improve clarity for new contributors.
- Updated `contributing_checks.rst` to improve clarity and consistency by replacing inline code formatting with Sphinx directives for better readability.
- Added a new `Makefile` to facilitate Sphinx documentation building, providing a minimal setup for users to generate documentation easily.
Copy link
Contributor

@stephprince stephprince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!


1. Open a :nwbinspector-issues:`'New Check' issue <>`
2. Describe what the check will validate
3. Link to relevant :doc:`best practices documentation <best_practices/best_practices_index>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
3. Link to relevant :doc:`best practices documentation <best_practices/best_practices_index>`
3. Link to relevant :doc:`best practices documentation <best_practices/best_practices_index>` if applicable. If proposing a new best practice, please describe in detail.

7. ``_ecephys.py`` - Extracellular electrophysiology
8. ``_ophys.py`` - Optical physiology
9. ``_ogen.py`` - Optogenetics
10. ``_image_series.py`` - :py:class:`~pynwb.image.ImageSeries` objects
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
10. ``_image_series.py`` - :py:class:`~pynwb.image.ImageSeries` objects
10. ``_image_series.py`` - :py:class:`~pynwb.image.ImageSeries` objects
11. ``_images.py`` - :py:class:`~pynwb.base.Images` objects
12. ``_general.py`` - attributes of general neurodata types

Comment on lines +10 to +11
:doc:`how to contribute new checks <contributing_checks>`. A guideline on how to build a new data interface and a graphical
overview of the object structure can be found in our primary :nwbinspector-contributing:`Contributing <>` page.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:doc:`how to contribute new checks <contributing_checks>`. A guideline on how to build a new data interface and a graphical
overview of the object structure can be found in our primary :nwbinspector-contributing:`Contributing <>` page.
:doc:`how to contribute new checks <contributing_checks>`.

The "Contributing" page is now a bit repetitive with the new contributing_checks.rst documentation, and I think "Contributing" should be more generalized than just adding a new check. Your proposed documentation is also probably easier for new contributors to find.

What do you think about making CONTRIBUTING.md focused on coding style and the code of conduct, and then moving any other "Adding a new check" information from CONTRIBUTING.md to contributing_checks.rst.

Comment on lines +62 to +64

4. Choose the Right Importance Level
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
4. Choose the Right Importance Level
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
.. note::
The function name for the check should always start with ``check_``
4. Choose the Right Importance Level
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Comment on lines +88 to +100

6. Best Practices for Check Implementation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1. Keep logic simple and focused
2. Use descriptive variable names
3. Add comments for complex logic
4. Reuse utility functions from :doc:`api/utils` when possible
5. Make error messages clear and actionable
6. Include links to relevant documentation in docstrings

7. Submit Your PR
^^^^^^^^^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
6. Best Practices for Check Implementation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1. Keep logic simple and focused
2. Use descriptive variable names
3. Add comments for complex logic
4. Reuse utility functions from :doc:`api/utils` when possible
5. Make error messages clear and actionable
6. Include links to relevant documentation in docstrings
7. Submit Your PR
^^^^^^^^^^^^^^^^^
6. Add Check to the Public Interface
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1. Add an import for your check from the appropriate module in ``src/nwbinspector/checks/__init__.py``
2. Add your check to the ```__all__``` list in ``src/nwbinspector/checks/__init__.py`` to indicate
the check is part of the public interface.
7. Add Check to the Documentation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Add a link to your new check function in the relevant best practice section of the ``docs/best_practices`` folder.
If needed, add a new section and label for your check:
.. code-block:: rst
.. _best_practice_my_feature:
My Feature
~~~~~~~~~~
Description of the best practice.
Check function: :py:meth:`~nwbinspector.checks._tables.check_my_feature`
.. note::
If the best practice label in the ``.rst`` file ends with the same pattern as the check function name,
(e.g. ``.. _best_practice_my_feature:`` and ``check_my_feature``), a link to the best practice documentation
will be automatically added to the function API documentation.
However, if the name of your check function does not match the name of your best practice section label
(e.g. if a single best practices section has multiple check functions), you can include a link in the function
docstring to link to the related best practice section.
.. code-block:: python
def check_my_feature(nwbfile: NWBFile) -> Optional[InspectorMessage]:
"""
One-line description of what this check validates.
Best Practice: :ref:`best_practice_my_feature_unique_label`
"""
7. Best Practices for Check Implementation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1. Keep logic simple and focused
2. Use descriptive variable names
3. Add comments for complex logic
4. Reuse utility functions from :doc:`api/utils` when possible
5. Make error messages clear and actionable
6. Include links to relevant documentation in docstrings
8. Submit Your PR
^^^^^^^^^^^^^^^^^

I came across this documentation docstring behavior recently and thought it might be helpful to describe here. This description is based on my understanding of the conf.py file, but feel free to update if I'm missing anything:

def add_refs_to_docstrings(app, what, name, obj, options, lines):
if what == "function" and obj.__name__.startswith("check_") and "Best Practice: " not in obj.__doc__:
lines.append(f"Best Practice: :ref:`best_practice_{obj.__name__.split('check_')[1]}`")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants