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: Add type hints and improve docstrings of load_tile_map function #3087

Merged
merged 7 commits into from Mar 10, 2024

Conversation

@seisman seisman added skip-changelog Skip adding Pull Request to changelog typing Type hints and static type checking needs review This PR has higher priority and needs review. labels Mar 5, 2024
@seisman seisman added this to the 0.12.0 milestone Mar 5, 2024
@seisman seisman removed the needs review This PR has higher priority and needs review. label Mar 5, 2024
@seisman seisman marked this pull request as draft March 5, 2024 05:41
@seisman seisman marked this pull request as ready for review March 5, 2024 06:05
@seisman seisman added the needs review This PR has higher priority and needs review. label Mar 5, 2024
@seisman seisman marked this pull request as draft March 5, 2024 06:20
@seisman seisman removed the needs review This PR has higher priority and needs review. label Mar 5, 2024
@seisman seisman added the needs review This PR has higher priority and needs review. label Mar 6, 2024
@seisman seisman marked this pull request as ready for review March 6, 2024 05:53
Comment on lines 90 to 93
Returns
-------
raster : xarray.DataArray
raster
Georeferenced 3-D data array of RGB values.
Copy link
Member

Choose a reason for hiding this comment

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

Now there are two sections, Return type and Returns, and the returned object is written in italic instead of bold style. I personally prefer the current documentation version, as I think the returned object and its data type should be stated together (analogous to the parameters).

current docs updated docs
tilemap_return_OLD tilemap_return_NEW

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 check if sphinx-autodoc-typehint provides an option to control the behavior. https://github.com/tox-dev/sphinx-autodoc-typehints#options

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 sphinx-autodoc-typehint provides such an option, so there is little we can do unless we decide not to add type hints for the return values.

Copy link
Member

@weiji14 weiji14 Mar 8, 2024

Choose a reason for hiding this comment

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

There was a napoleon_use_rtype = True option that was tried in #937. Does it work here, or is that what sphinx-autodoc-typehint is using?

Copy link
Member

Choose a reason for hiding this comment

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

Can you try napoleon_preprocess_types = True as suggested at sphinx-doc/sphinx#9394?

Copy link
Member Author

Choose a reason for hiding this comment

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

napoleon_use_rtype = True incorrectly parses raster as the return type:
Screenshot from 2024-03-08 08-27-45

napoleon_preprocess_types = True and napoleon_use_rtype = False is not bad:

Screenshot from 2024-03-08 08-29-06

Copy link
Member Author

@seisman seisman Mar 8, 2024

Choose a reason for hiding this comment

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

https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#confval-napoleon_preprocess_types

napoleon_preprocess_types
True to convert the type definitions in the docstrings as references. Defaults to False.

When setting napoleon_preprocess_types = True, raster is parsed as type definitions, so it's rendered the same way as the return type DataArray. Strictly speaking, it's still incorrect.

Actually, the name of the returned value should not appear in the NumPy-style docstrings (see https://www.sphinx-doc.org/en/master/usage/extensions/example_numpy.html#example-numpy). I.e., we should change

    Returns
    -------
    raster
        Georeferenced 3-D data array of RGB values.

to

    Returns
    -------
        Georeferenced 3-D data array of RGB values.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the name of the returned value should not appear in the NumPy-style docstrings (see https://www.sphinx-doc.org/en/master/usage/extensions/example_numpy.html#example-numpy). I.e., we should change

It looks like they are using something like

    Returns
    -------
    xr.DataArray
        Georeferenced 3-D data array of RGB values.

Copy link
Member Author

Choose a reason for hiding this comment

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

That version doesn't have type hints for the return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

seisman and others added 2 commits March 7, 2024 17:07
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
@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. labels Mar 7, 2024
@seisman
Copy link
Member Author

seisman commented Mar 10, 2024

I'm merging this PR. The style issue of the function return value/type should be addressed separately (if there is a good solution).

@seisman seisman merged commit 28e3513 into main Mar 10, 2024
18 of 19 checks passed
@seisman seisman deleted the typing/tile_map branch March 10, 2024 11:19
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Mar 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Skip adding Pull Request to changelog typing Type hints and static type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants