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

DOC: API reference tweaks & clean-up #89

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

benbovy
Copy link
Owner

@benbovy benbovy commented Dec 10, 2024

Mostly cosmetic changes to make the API reference documentation nicer and more consistent across the library.

More details in each commit message.

Also includes changes suggested in #87.

Add them manually instead, without type hints. It is a bit more tedious
to maintain but the result is more consistent and readable.

- Pybind11 type hints not always reliable (e.g., non-vectorized
arguments of py::vectorize functions show as numpy.ndarray, too generic
return type)

- didn't work with sphinx's ``autodoc_typehints = "none"``, which seemed
to be ignored for some reason. Adding type hints in the function
signatures in the API documentation makes those signature less readable
and doesn't add much information (type hints already in the parameter
and return description sections)

Also add "Returns" sections in docstrings more globally.
Return values that are consistent with shapely for collections.
Also move Projection to api-hidden.
Also move get_x and get_y functions to geography.cpp (geography
properties).
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 74.75728% with 26 lines in your changes missing coverage. Please review.

Project coverage is 86.36%. Comparing base (38a0693) to head (6c506a1).
Report is 44 commits behind head on main.

Files with missing lines Patch % Lines
src/geography.cpp 78.37% 8 Missing ⚠️
src/io.cpp 68.00% 8 Missing ⚠️
src/accessors-geog.cpp 50.00% 6 Missing ⚠️
src/geoarrow.cpp 80.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #89      +/-   ##
==========================================
- Coverage   94.00%   86.36%   -7.64%     
==========================================
  Files           5       13       +8     
  Lines         200      814     +614     
  Branches        8       63      +55     
==========================================
+ Hits          188      703     +515     
- Misses          5       81      +76     
- Partials        7       30      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Great work!

I added some comments while going through the individual commits (thanks for the nice separate commits and messages, that certainly made it easier to review!), but you can see what you address here.

docs/conf.py Outdated Show resolved Hide resolved
src/geoarrow.cpp Outdated Show resolved Hide resolved
src/geoarrow.cpp Outdated Show resolved Hide resolved
src/geography.cpp Outdated Show resolved Hide resolved
src/accessors-geog.cpp Outdated Show resolved Hide resolved
src/geography.cpp Show resolved Hide resolved
src/predicates.cpp Outdated Show resolved Hide resolved
src/projections.cpp Outdated Show resolved Hide resolved
src/geography.cpp Outdated Show resolved Hide resolved
src/accessors-geog.cpp Show resolved Hide resolved
@benbovy
Copy link
Owner Author

benbovy commented Jan 7, 2025

@jorisvandenbossche I addressed your review comments in the last two commits. Do you want to review it before merging this? Ortherwise this is ready I think.

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.

2 participants