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

Add docstrings to sliders module #3176

Merged
merged 7 commits into from Mar 30, 2022
Merged

Add docstrings to sliders module #3176

merged 7 commits into from Mar 30, 2022

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Feb 6, 2022

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

  • I've used Markdown in docstrings because they are simple, known to lots of people, render nicely in VS Code.
  • For each slider I've added a link to the reference gallery notebook. It will speed up navigation and learning so much in VS Code.
  • For each slider I've added a small, working code example in a fenced code block. Again being able to see and copy-paste such an example easily in VS Code will just speed up development so much I believe.

But this is my opinion. Please review and provide yours. Thanks.

Example

vscode.mp4

panel/widgets/slider.py Outdated Show resolved Hide resolved
panel/widgets/slider.py Outdated Show resolved Hide resolved
panel/widgets/slider.py Outdated Show resolved Hide resolved
panel/widgets/slider.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Feb 6, 2022

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 Show resolved Hide resolved
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?
Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks

panel/widgets/slider.py Outdated Show resolved Hide resolved
panel/widgets/slider.py Outdated Show resolved Hide resolved
The selected date value of the slider. Updated when the slider handle is dragged. Supports
datetime.datetime, datetime.date or np.datetime64 types.""")


Copy link
Member

Choose a reason for hiding this comment

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

Remove extra space

panel/widgets/slider.py Outdated Show resolved Hide resolved
@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Feb 7, 2022

@maximlt . Regarding your question on Sphinx docs.

No. I just know @jbednar stated he support use of Markdown.

I don't believe its documented anywhere how to build the documentation locally to be able to check it out.

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Feb 7, 2022

Thanks. I've updated based on the suggestions from @maximlt and @hoxbro

The Class docstrings now look like

image

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)

??

@maximlt
Copy link
Member

maximlt commented Feb 7, 2022

@maximlt . Regarding your question on Sphinx docs.

No. I just know @jbednar stated he support use of Markdown.

I don't believe its documented anywhere how to build the documentation locally to be able to check it out.

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 versionadded or deprecated.

@maximlt
Copy link
Member

maximlt commented Feb 7, 2022

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!

@mattkram
Copy link

mattkram commented Mar 28, 2022

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
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #3176 (6d83ac6) into master (0c4408b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
panel/widgets/slider.py 84.16% <100.00%> (+0.03%) ⬆️
panel/io/reload.py 69.56% <0.00%> (-2.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c4408b...6d83ac6. Read the comment docs.

@philippjfr philippjfr merged commit 1acb03f into master Mar 30, 2022
@philippjfr philippjfr deleted the slider-docstrings branch March 30, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants