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.

Sign up for GitHub

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

Jump to bottom

TYP: Improve the doc style for type hints #2813

Merged
merged 9 commits into from
Dec 7, 2023
Merged

TYP: Improve the doc style for type hints #2813

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cc6c461
Add partial type hints support to load_earth_relief
seisman Nov 2, 2023
5c5864c
Set autodoc_typehints to description
seisman Nov 15, 2023
9ceacb9
Use the sphinx-autodoc-typehints extension
seisman Nov 15, 2023
c7be7b1
Remove autodoc_typehints and set typehints_defaults to 'comma'
seisman Nov 15, 2023
66c4e69
Merge branch 'main' into typehints/doc-style
seisman Dec 4, 2023
7a3189a
Update environment.yml
seisman Dec 4, 2023
15b2c07
Merge branch 'main' into typehints/doc-style
seisman Dec 6, 2023
f3c1b19
Revert "Add partial type hints support to load_earth_relief"
seisman Dec 6, 2023
4bc5912
Merge branch 'main' into typehints/doc-style
yvonnefroehlich Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci_docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ jobs:
myst-parser
panel
sphinx
sphinx-autodoc-typehints
sphinx-copybutton
sphinx-design
sphinx-gallery
Expand Down
1 change: 1 addition & 0 deletions ci/requirements/docs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ dependencies:
- myst-parser
- panel
- sphinx
- sphinx-autodoc-typehints
- sphinx-copybutton
- sphinx-design
- sphinx-gallery
Expand Down
4 changes: 4 additions & 0 deletions doc/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
"sphinx.ext.napoleon",
"sphinx_autodoc_typehints",
"sphinx_copybutton",
"sphinx_design",
"sphinx_gallery.gen_gallery",
Expand All @@ -41,6 +42,9 @@
napoleon_use_rtype = False
napoleon_use_ivar = True

# sphinx_auto_typehints
typehints_defaults = "comma"

# configure links to GMT docs
extlinks = {
"gmt-docs": ("https://docs.generic-mapping-tools.org/6.4/%s", None),
Expand Down
1 change: 1 addition & 0 deletions environment.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ dependencies:
- myst-parser
- panel
- sphinx
- sphinx-autodoc-typehints
- sphinx-copybutton
- sphinx-design
- sphinx-gallery
Expand Down
15 changes: 9 additions & 6 deletions pygmt/datasets/earth_relief.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

The grids are available in various resolutions.
"""
from collections.abc import Sequence
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to revert any changes in this file before merging.

from typing import Literal, Union

from pygmt.datasets.load_remote_dataset import _load_remote_dataset
from pygmt.exceptions import GMTInvalidInput
from pygmt.helpers import kwargs_to_strings
Expand All @@ -14,9 +17,9 @@
@kwargs_to_strings(region="sequence")
def load_earth_relief(
resolution="01d",
region=None,
registration=None,
data_source="igpp",
region: Union[str, Sequence, None] = None,
registration: Literal["gridline", "pixel", None] = None,
Copy link
Member

Choose a reason for hiding this comment

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

I feel the current doc style (style 4 in the above table) is the best one.

I agree that the signature of No 1 is too complicated. The highlighting of No 3 and No 4 is good, as it is consistent with the rest of the documentation.
Is it required to always give a default value? I am wondering if something like this can be a bit misleading (even though it is also given like this in the signature):
283153676-d904cd92-67f1-4306-a73a-775a79229b4a_cut_default

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it required to always give a default value?

We don't have to give a default value, but I feel a default value in the function strings is good so that users don't have to go back to the function signature to know the default.

I am wondering if something like this can be a bit misleading (even though it is also given like this in the signature): 283153676-d904cd92-67f1-4306-a73a-775a79229b4a_cut_default

Yes, it's misleading. We can rephrase it to something like:

Default is None, which means "gridline" for all resolutions except ...

Copy link
Member

@yvonnefroehlich yvonnefroehlich Dec 5, 2023 •
edited

Choose a reason for hiding this comment

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

We don't have to give a default value, but I feel a default value in the function strings is good so that users don't have to go back to the function signature to know the default.

I also think it's good to have the default value in the function string. Just thought about not stating it for cases like above, but rephrasing the docstring should also prevent confusion.

data_source: Literal["igpp", "gebco", "gebcosi", "synbath"] = "igpp",
Copy link
Member

Choose a reason for hiding this comment

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

It is probably not possible to get:

  • double quotation marks (" ") instead of single quotation marks (' ')
  • an upper-case Default

283153676-d904cd92-67f1-4306-a73a-775a79229b4a_cut

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's possible.

use_srtm=False,
):
r"""
Expand Down Expand Up @@ -63,18 +66,18 @@ def load_earth_relief(
``"04m"``, ``"03m"``, ``"02m"``, ``"01m"``, ``"30s"``, ``"15s"``,
``"03s"``, or ``"01s"``.

region : str or list
region
The subregion of the grid to load, in the form of a list
[*xmin*, *xmax*, *ymin*, *ymax*] or a string *xmin/xmax/ymin/ymax*.
Required for Earth relief grids with resolutions higher than 5
arc-minutes (i.e., ``"05m"``).

registration : str
registration
Grid registration type. Either ``"pixel"`` for pixel registration or
``"gridline"`` for gridline registration. Default is ``"gridline"``
for all resolutions except ``"15s"`` which is ``"pixel"`` only.

data_source : str
data_source
Select the source for the Earth relief data. Available options are:

- ``"igpp"``: IGPP Earth Relief [Default option]. See
Expand Down