Add module docstring to dandiapi.py #756
Conversation
Codecov Report
@@ Coverage Diff @@
## master #756 +/- ##
==========================================
- Coverage 84.85% 84.83% -0.02%
==========================================
Files 59 59
Lines 6021 6027 +6
==========================================
+ Hits 5109 5113 +4
- Misses 912 914 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
dandi/dandiapi.py
Outdated
|
|
||
| .. code:: python | ||
|
|
||
| from operator import attrgetter |
There was a problem hiding this comment.
| from operator import attrgetter |
since no longer used.
Please make this code snippet/docstring doc tested so we could ensure it stays valid.
With that let's finalize this PR and let's get it merged.
There was a problem hiding this comment.
@yarikoptic The code downloads assets. How is that supposed to be safely doctested (to say nothing of how long it could take to run the code)?
There was a problem hiding this comment.
d'oh , right. Lets may be comment out the download line (with a comment to use it if download is desired) and just print metadata? I just feel that having at least majority of the docstring sample tested would be of great benefit long term
There was a problem hiding this comment.
@yarikoptic Doctests work by comparing the code's output to the output given in the doctest. Aside from the fact that most Dandiset metadata is multiple lines long even when not formatted, we would have to update the doctest whenever a new version is published.
There was a problem hiding this comment.
could we use # doctest:+ELLIPSIS and have ... to swallow all output?
There was a problem hiding this comment.
@yarikoptic That works, but the doctest adds about 50 seconds to the total test runtime.
There was a problem hiding this comment.
that is indeed notable. Could we may be run such a full sweep with doc-tests only on cron (e.g. daily)?
There was a problem hiding this comment.
Looks good to me. Time to undraft and merge?
|
Great, Thank you, let's proceed. |
Closes #755.