Add docstring to add_recording

[h-mayorquin]

add_recording is not showing up in the API somehow. I am wondering if it is the lack of docstring...

anyway, a docstring is good to add anyway so adding it here.

[h-mayorquin]

This does add add_recording to the API. So it seems that the auto-docs only works when you have a docstring.

[CodyCBakerPhD]

This does add add_recording to the API. So it seems that the auto-docs only works when you have a docstring.

Yeah, we've known that for a while

You should have a chat with @pauladkisson sometime; we solved this problem on ROIExtractors by creating a custom pre-commit that ensures all publically exposed functions and classes have docstrings (which include from inheritance)

[h-mayorquin]

I don't really like that solution : P

I remember finding the pre-commit annoying in roi-extractors. Makes me less likely to want to contribute. It was something about enforcing spaces that did not make a lot of sense to me.

I will have to thing again.

[CodyCBakerPhD]

I think you're talking about a separate pre-commit for pydocstyle which ensures strict numpy style compliance

The check for ensuring public functions have a non-empty docstring is its own thing (so that help(< some method >) or < some method >? doesn't return < no docstring >) and even if annoying is valuable for both the API docs (which chatbots can then scrub) and general usage

Have you tried writing docstring using GitHub copilot? For me it formats everything perfectly, takes less than 15 seconds per method, and gets it almost perfect in the descriptions. So nowadays I don't think the cost is high at all to enforce

[pauladkisson]

we solved this problem on ROIExtractors by creating a custom pre-commit that ensures all publically exposed functions and classes have docstrings (which include from inheritance)

Actually, on roiextractors it's a custom github workflow (not pre-commit) that ensures all publicly exposed functions and classes have docstrings. So, it just has to be satisfied before merging, which is really not much of a burden at all.

[h-mayorquin]

I stand corrected, thanks for explaining it to me guys. I will check it out.

[bendichter]

I think it makes sense to enforce that all functions and methods must either be marked as private with a _ or have a docstring, especially since it appears this is critical for autodoc to work.

For modules, I have found it annoying and often repetitive to need to create docstrings for basic __init__.py files. Would it be possible to omit these?

(duplicated here: catalystneuro/.github#17)

[CodyCBakerPhD]

For modules, I have found it annoying and often repetitive to need to create docstrings for basic init.py files. Would it be possible to omit these?

See catalystneuro/.github#17 (comment) part 2 for full details

But in summary, if we do a better job of controlling what we publically expose as submodules then we can indeed ignore the majority of __init__.py files once their parent submodules are marked as private

[h-mayorquin]

@CodyCBakerPhD @bendichter

Yeah, we've known that for a while

I think it makes sense to enforce that all functions and methods must either be marked as private with a _ or have a docstring, especially since it appears this is critical for autodoc to work.

Not that it has any bearing on what is desirable but note that with the changes in #1011 functions no longer require a docstring for autodoc to display then. That was a configuration option not a requirement.

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-option-automodule-undoc-members

Check out here:
https://neuroconv.readthedocs.io/en/main/api/tools.spikeinterface.html#neuroconv.tools.spikeinterface.spikeinterface.add_recording

Now it is displayed even if this has not been merged.

[bendichter]

@h-mayorquin thanks that's helpful to know. I still think we should require docstrings for non-private methods.

[h-mayorquin]

@h-mayorquin thanks that's helpful to know. I still think we should require docstrings for non-private methods.

Agree, will check out the solution of roiextractors mentioned above.

[h-mayorquin]

So, is the docstring OK, can we merge this?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.25%. Comparing base (b9507bf) to head (111cbfd).
Report is 96 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1010      +/-   ##
==========================================
- Coverage   91.43%   91.25%   -0.19%     
==========================================
  Files         127      127              
  Lines        7540     7555      +15     
==========================================
  Hits         6894     6894              
- Misses        646      661      +15     

Flag	Coverage Δ
unittests	91.25% <ø> (-0.19%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...c/neuroconv/tools/spikeinterface/spikeinterface.py	95.10% <ø> (-0.02%)	⬇️