-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DOC: Add missing docstrings for Index.empty, Index.view and Index.names #57546
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
Conversation
merlinymy
commented
Feb 21, 2024
- [ All docstrings tests passed]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @merlinymy, great work. Can you have a look at the problems in the continuous integration and make sure everything is green, so we can review and merge this please? Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the tailing whitespaces and the see also section with a private method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More fixes to the docstring, so we can get it merged (author doesn't seem responsive, but there is a lot of nice work here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More fixes, removing tailing whitespaces and adding blank line between sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed all the issues here, if someone else can have a second look to see if this can be merge. The PR is stale but it's a nice addition, so I've been addressing all the issues myself.
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
/preview |
LGTM. |
@mroeschke all docstrings seem to have already been added in main, should we close this? I think the changes here provide more info and I also see a few differences that look correct here but are wrong on main. I can follow up with a PR that will add some of the changes here to the current docstrings. |
Sure we can close this and have a follow up to improve the docstrings if needed |