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

TYP: Improve the doc style for type hints #2813

Merged
merged 9 commits into from
Dec 7, 2023
Merged

Conversation

seisman
Copy link
Member

@seisman seisman commented Nov 15, 2023

In commit cc6c461, I added type hints to the region, registration and data_source parameters and also remove the parameter types from docstrings (following this guide). Other parameters like resolution and use_srtm are not changed. So now this function has "partial" type hints support.

This PR tries to find out the best style for API documentations.

Preview: https://pygmt-dev--2813.org.readthedocs.build/en/2813/api/generated/pygmt.datasets.load_earth_relief.html

No Signature Docstrings Comment
1 Default image image Function signature is too complicated to read
2 5c5864c image image Function signature is easy to read, but the link-style types (e.g., str) are a little distractive
3 9ceacb9 image image Types are still clickable but not as destructive as style 2
4 c7be7b1 image image Similar to style 3, but it also automatically show the default value

I feel the current doc style (style 4 in the above table) is the best one. What do you think @GenericMappingTools/pygmt-maintainers?

@seisman seisman added the typing Type hints and static type checking label Nov 15, 2023
@seisman seisman force-pushed the typehints/doc-style branch from 02e0baf to cc6c461 Compare November 15, 2023 09:33
@seisman seisman added the documentation Improvements or additions to documentation label Nov 16, 2023
@seisman seisman added this to the 0.11.0 milestone Nov 16, 2023
@seisman seisman added maintenance Boring but important stuff for the core devs and removed documentation Improvements or additions to documentation labels Nov 16, 2023
@seisman seisman marked this pull request as ready for review December 4, 2023 12:15
@seisman seisman added the needs review This PR has higher priority and needs review. label Dec 4, 2023
@@ -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.

environment.yml Outdated Show resolved Hide resolved
@seisman seisman requested a review from a team December 4, 2023 16:33
Comment on lines 20 to 22
region: Union[str, Sequence, None] = None,
registration: Literal["gridline", "pixel", None] = None,
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.

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

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.

@seisman seisman requested a review from a team December 7, 2023 09:37
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. final review call This PR requires final review and approval from a second reviewer labels Dec 7, 2023
@seisman seisman merged commit 7845ae8 into main Dec 7, 2023
17 checks passed
@seisman seisman deleted the typehints/doc-style branch December 7, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs typing Type hints and static type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants