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

Improved module level docstrings #793

Merged
merged 18 commits into from
Jul 31, 2022
Merged

Improved module level docstrings #793

merged 18 commits into from
Jul 31, 2022

Conversation

MarcSkovMadsen
Copy link
Collaborator

@MarcSkovMadsen MarcSkovMadsen commented Jul 24, 2022

Addresses a small part of #789 by adding module level docstrings.

The aim here is to provide

  • an elevator pitch for hvplot
  • a very basic intro and reminder of the api
  • info enough to easily navigate hvplot and its resources from the editor.

I believe all the hvplot.pandas, hvplot.xarray, hvplot.ibis needs docstrings that are similar but specialized to the specific data source. These are the entry points for users. Not the top-level hvplot module.

There is one issue though. VS Code does not show docstring tooltips for nested imports like import hvplot.pandas. It only shows for from hvplot import pandas. See microsoft/pylance-release#3093.

from-hvplot-import-pandas (1)

hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Jul 24, 2022

Thanks Marc!

There is one issue though. VS Code does not show docstring tooltips for nested imports like import hvplot.pandas. It only shows for from hvplot import pandas. See microsoft/pylance-release#3093.

Oh I would not advise anyone to make such an import! from hvplot import pandas is confusing as it gives the impression you're importing pandas. Until the Pylance issue is resolved I don't see much point in adding high-level pitch docstring to the modules like pandas.py.

(I think the tests are failing because of the latest xarray)

MarcSkovMadsen and others added 5 commits July 25, 2022 06:31
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
Co-authored-by: Maxime Liquet <35924738+maximlt@users.noreply.github.com>
@MarcSkovMadsen
Copy link
Collaborator Author

Thanks Marc!

There is one issue though. VS Code does not show docstring tooltips for nested imports like import hvplot.pandas. It only shows for from hvplot import pandas. See microsoft/pylance-release#3093.

Oh I would not advise anyone to make such an import! from hvplot import pandas is confusing as it gives the impression you're importing pandas. Until the Pylance issue is resolved I don't see much point in adding high-level pitch docstring to the modules like pandas.py.

(I think the tests are failing because of the latest xarray)

I still see much value :-)

  • Modules should have docstrings. Its best practice and helpful.
  • It still creates value in Jupyter and VS Code as shown below.
  • You have someone willing to contribute the docstrings now, so why not just get them in?

Before

image
image

After

image
image
If you go to definition you will also find the documentation
image

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Jul 25, 2022

I believe the intro section, how to .. in 3 simple steps and help is the most important sections. I have simplified and removed the rest.

Thanks for the feedback @maximlt . Let me know if there is more I should do? Thanks.

Copy link
Member

@jbednar jbednar 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; thanks so much!

hvplot/__init__.py Outdated Show resolved Hide resolved
hvplot/__init__.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Jul 29, 2022

Thanks @MarcSkovMadsen for the updates and taking into account my suggestions!

I've just realized that starting from Python 3.7 modules can implement __getattr__ and __dir__ (https://peps.python.org/pep-0562/). This means we could easily get rid of all of the shim modules that were required before with a cleaner approach, and this when hvPlot will require 3.7 as a minimum version (now it's 3.6 but I guess not for too long). This means that the shim module doctsrings, like the one you added in pandas.py, will go away with this new implementation. I'm sorry for the extra work this caused you but I'd suggest you already remove them in this PR? I felt they were anyway very close to the init.py module docstring which I already find very useful!

Marc Skov Madsen and others added 3 commits July 29, 2022 17:03
Co-authored-by: James A. Bednar <jbednar@users.noreply.github.com>
Co-authored-by: James A. Bednar <jbednar@users.noreply.github.com>
@MarcSkovMadsen
Copy link
Collaborator Author

I've removed the shim module docstrings @maximlt.

Why do we actually not do hvplot.extension("pandas") or hvplot.extension("xarray") similarly to pn.extension and hv.extension('matplotlib')?

That would be consistent with the rest of the ecosystem and work better with static type checking tools.

@maximlt
Copy link
Member

maximlt commented Jul 29, 2022

I've removed the shim module docstrings

Thanks!

Why do we actually not do hvplot.extension("pandas") or hvplot.extension("xarray") similarly to pn.extension and hv.extension('matplotlib')?

You can run hvplot.extension('matplotlib') to set the Matplotlib backend, which is consistent with HoloViews. I don't know whether allowing hvplot.extension('pandas') would make it more consistent with the ecosystem. I find hv.extension() and pn.extension() quite confusing, too implicit and possibly overloaded. We see users calling them without really knowing what they do, that might be a documentation issue, I don't know.

work better with static type checking tools.

How?

@maximlt
Copy link
Member

maximlt commented Jul 29, 2022

I've removed the shim module docstrings @maximlt.

Arf actually I see they're still around. Did you maybe forget to commit?

@MarcSkovMadsen
Copy link
Collaborator Author

I've pushed the changes now @maximlt

@maximlt
Copy link
Member

maximlt commented Jul 31, 2022

There were still two weird files on your branch. I took the freedom to delete them, they seemed totally unrelated to that PR. Merging, thanks Marc!

@maximlt maximlt merged commit 5ad03b3 into master Jul 31, 2022
@maximlt maximlt deleted the feature/hvplot-tooltip branch July 31, 2022 08:19
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.

None yet

3 participants