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

add sphere to the docs #118

Conversation

philippemiron
Copy link
Contributor

No description provided.

@philippemiron
Copy link
Contributor Author

Maybe, let's keep it open until @milancurcic sees it because I might be forgetting other things.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

It's correctly included in the API docs index.

However, do check one thing: I noticed that your format of docstrings (e.g. like the one you used here) are not properly rendered by Sphinx. I don't know if it's a missing Sphinx setting or just a bad format. In contrast, look at the dataformat.unpack_ragged docstring where I used the NumPy style format and how it's rendered.

I think this PR should just go ahead as is, but in a separate PR we need to go over all docstrings and fix them up. GitHub Copilot is now pretty good at autogenerating them. I suggest just following the NumPy style guide for docstrings.

@philippemiron
Copy link
Contributor Author

I'll take a look later. I can probably go through all the files and do something uniform.

@philippemiron
Copy link
Contributor Author

ps: my branch was not up-to-date with main, so I guess docstring should be fixed in another PR as you suggested. Also, I'm not sure how to proceed, for example it looks like you wrote: array-like as type of parameters, and I was using directly the same keyworkd that I used for typing..

@Cloud-Drift Cloud-Drift deleted a comment from codecov bot Feb 20, 2023
@milancurcic
Copy link
Member

Also, I'm not sure how to proceed, for example it looks like you wrote: array-like as type of parameters, and I was using directly the same keyworkd that I used for typing..

Yes, I wasn't 100% sure what to do about this either. I followed NumPy doc style to use array-like to indicate that the function will work with np.ndarray and xr.DataArray alike. Perhaps we can just be explicit and say np.ndarray | xr.DataArray, although I think this type syntax isn't supported before Python 3.10. Something to check.

Either way, the type hints in the docstring don't need to exactly match those in the function definition. The ones in the function definition are for IDEs to parse, and the ones in the docstring merely need to make sense to the reader.

I think we just need to pick what makes sense for us in this project, and be consistent with it.

@milancurcic milancurcic merged commit 8669dba into Cloud-Drift:main Feb 20, 2023
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