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

Improve API documentation in CompositeBase #1968

Merged
merged 3 commits into from Jan 18, 2022
Merged

Improve API documentation in CompositeBase #1968

merged 3 commits into from Jan 18, 2022

Conversation

gerritholl
Copy link
Collaborator

@gerritholl gerritholl commented Jan 7, 2022 •
edited

Improve the API documentation in CompositeBase. Correct the spelling of
the NEGLIGIBLE_COORDS module attribute. Remove the spelling NEGLIBLE_COORDS.
Also remove the check_areas method, which had been deprecated for a while.

  • Closes #xxxx
  • Tests added
  • Fully documented

@gerritholl
Improve API documentation in CompositeBase
f5c45a7 
Improve the API documentation in CompositeBase.  Correct the spelling of
the NEGLIGIBLE_COORDS module attribute (keeping the old NEGLIBLE_COORDS
in place for backward compatibility purposes).

Mark check_areas as deprecated in the docstring.
@codecov
Copy link

codecov bot commented Jan 7, 2022 •
edited

Codecov Report

Merging #1968 (347a435) into main (279631e) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 347a435 differs from pull request most recent head 7cc9443. Consider uploading reports for the commit 7cc9443 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1968      +/-   ##
==========================================
- Coverage   93.53%   93.51%   -0.02%     
==========================================
  Files         279      279              
  Lines       41312    41309       -3     
==========================================
- Hits        38642    38632      -10     
- Misses       2670     2677       +7
Flag Coverage Δ
behaviourtests 4.77% <100.00%> (-0.01%) ⬇️
unittests 94.05% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/composites/__init__.py 89.76% <100.00%> (+0.22%) ⬆️
satpy/tests/reader_tests/test_vii_l2_nc.py 93.93% <0.00%> (-6.07%) ⬇️
satpy/tests/reader_tests/test_vii_l1b_nc.py 97.18% <0.00%> (-2.82%) ⬇️
satpy/tests/reader_tests/test_nwcsaf_msg.py 96.63% <0.00%> (-1.69%) ⬇️
satpy/tests/reader_tests/test_vii_base_nc.py 98.75% <0.00%> (-1.25%) ⬇️
satpy/tests/test_config.py 93.84% <0.00%> (-0.77%) ⬇️

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 279631e...7cc9443. Read the comment docs.

@coveralls
Copy link

coveralls commented Jan 7, 2022 •
edited

Coverage Status

Coverage increased (+0.004%) to 94.003% when pulling 7cc9443 on gerritholl:improve-composite-docs into 279631e on pytroll:main.

djhoese
djhoese requested changes Jan 7, 2022
View reviewed changes
satpy/composites/__init__.py Outdated Show resolved Hide resolved
satpy/composites/__init__.py Outdated Show resolved Hide resolved
satpy/composites/__init__.py
:func:`satpy.utils.unify_chunks`).

Args:
data_arrays (List[arrays]): Arrays to be checked
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we never made a decision on type annotations versus docstring types did we? @mraspaud @sfinkens @pnuu any opinions?

def func(a: int, b: list[str]) -> list[int]:
    """Some docstring.

    Args:
        a: description
        b: description

    Returns:
        description
    """

versus only putting them in the docstring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just tested locally. If I put the types only in the annotations, then the sphinx documentation reports the argument types in the signature, but not in the documentation body. The return type it documents in both locations. Maybe this is configurable, but if omitted in the docstring I think is not an improvement. Or would you put the information in both locations, duplicating it?

Copy link
Member

Choose a reason for hiding this comment

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

There may be a sphinx extension we need to make it handle the arguments (I would hope it would come out of the box in the future). I'd prefer not duplicating.

Copy link
Member

Choose a reason for hiding this comment

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

The logical place to put it is as type annotations of course, but sphinx not picking it up for the docstring is annoying of course. Can this help? https://github.com/tox-dev/sphinx-autodoc-typehints

Copy link
Member

Choose a reason for hiding this comment

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

or this https://sphinx-toolbox.readthedocs.io/en/latest/extensions/more_autodoc/typehints.html

Copy link
Member

Choose a reason for hiding this comment

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

Looking at xarray it looks like they tell "autodoc" to not include type hints and they have type annotations and docstring types in the source code. That's probably not what they want long term though and may just be a work around. Looks like they also have aliases for types/functions so they don't have to use long names in their docs when linking to things.

https://github.com/pydata/xarray/blob/main/doc/conf.py#L112

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So what is the conclusion here? Should I change this to have the type only in annotations?

Copy link
Member

Choose a reason for hiding this comment

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

Leave it as-is for now. It isn't how I would do it, but that definitely doesn't make it correct. We'll have to look at this in some other PR as far as the sphinx docs handling it correctly. If you are bored feel free to try one or more of them out and see how it renders if you only include type annotations and nothing in the docstring.

I'll merge this if there are no more comments.

satpy/composites/__init__.py Show resolved Hide resolved
@gerritholl
Remove NEGLIBLE_COORDS attribute and check_areas method
347a435 
Remove the misspelt and hopefully externally unused NEGLIBLE_COORDS module
attribute, and remove the check_areas method that had been deprecated
for a while.
@gerritholl
Point out that CompositeBase is also for modifiers
7cc9443 
In the docstring of CompositeBase, point out that all modifier classes
/also/ derive from CompositeBase.
@djhoese djhoese merged commit cccbe06 into pytroll:main Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djhoese djhoese djhoese approved these changes

@mraspaud mraspaud Awaiting requested review from mraspaud mraspaud is a code owner

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gerritholl @coveralls @mraspaud @djhoese