-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
Add docstrings to sliders module #3176
Conversation
Thanks Marc for this! Do you have any clue on how the autodocumented API would render with these docstrings including Markdown? Do you know if there are any changes to make to the doc build to render Markdown (sphinx settings?)? |
panel/widgets/slider.py
Outdated
value_throttled = param.Parameter(constant=True, doc="""The value of the slider. Updated when | ||
the handle is released""") | ||
|
||
# Why do we have both a format and formatter parameter? |
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.
Just adding a comment here for now so this is not going to be lost.
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.
Thanks
The selected date value of the slider. Updated when the slider handle is dragged. Supports | ||
datetime.datetime, datetime.date or np.datetime64 types.""") | ||
|
||
|
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.
Remove extra space
Thanks. I've updated based on the suggestions from @maximlt and @hoxbro The Class docstrings now look like I believe its very, very useful. We could also change Reference: https://panel.holoviz.org/reference/widgets/EditableRangeSlider.html to [Reference](https://panel.holoviz.org/reference/widgets/EditableRangeSlider.html) ?? |
I believe it would be necessary to see how the embedded Markdown renders in the autodocumented API reference. I think the last time I checked the integration wasn't great, but I may be wrong so it's worth trying. The reference to build Panel's documentation is in the relevant Github Action workflow (I've never tried to build it locally!). I know that personally I never look at the API page on the site since the Reference gallery already details the signature of Panel's components (and I have easy access to Panel's source code). Yet with a little bit more love the API page could be made more useful, so I'd really like to make sure the Markdown you intend to add integrates well and doesn't prevent us from using nice Sphinx directives like |
Something worth noting for the links we plan to embed in the docstrings is that Panel's website is the documentation of the latest version and we don't yet have a system to keep the website of previous versions. So someone using let's say Panel 0.13 in the future maybe be directed to the site that will be then at version 2.6. There isn't much that can be done in this PR against that though! |
I was going to recommend looking into the MyST project regarding rendering Markdown docstrings, but it is unfortunately not currently possible. Here is a GitHub issue for reference: executablebooks/MyST-Parser#228 It may be a good option should that become available in the future. |
Codecov Report
@@ Coverage Diff @@
## master #3176 +/- ##
==========================================
- Coverage 83.29% 83.28% -0.01%
==========================================
Files 195 195
Lines 26166 26167 +1
==========================================
- Hits 21795 21794 -1
- Misses 4371 4373 +2
Continue to review full report at Codecov.
|
A first step into solving #3131
An isolated PR for adding docstrings to the sliders module. I can use the feedback from this review if/ when I add more docstrings.
Please note some basic concepts
But this is my opinion. Please review and provide yours. Thanks.
Example
vscode.mp4