-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
STY: Use ruff to format docstrings #56863
Conversation
ci/code_checks.sh
Outdated
MSG='Partially validate docstrings (EX03)' ; echo $MSG | ||
$BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=EX03 --ignore_functions \ | ||
pandas.Series.plot.line \ |
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.
are you sure this can be removed? ruff
only works statically, so only checks generated docstrings - any docstring with a substitution won't be checked. that's what validate_docstrings.py
does
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.
Ah good point. Yeah none of the dynamic docstrings were able to get formatted so I'll revert this
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.
one more reason to get rid of dynamic docstrings 😉
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.
Big +1 to remove them!
>>> s = pd.Series({{'Corn Flakes': 100.0, 'Almond Delight': 110.0, | ||
... 'Cinnamon Toast Crunch': 120.0, 'Cocoa Puff': 110.0}}) | ||
>>> s = pd.Series( | ||
... [100.0, 110.0, 120.0, 110.0], |
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.
If ruff can't handle {{
, it might be better to open an issue at ruff instead of re-writing the documentation.
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.
True but IMO I think it's okay to rewrite these ones for now just to avoid the potential {{
confusion for future contributors
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.
lgtm - though I did not exhaustive check every change. Almost everything I see is overwhelmingly positive. The only negative I saw is that some examples became a bit long, and might be good to shorten at some point.
Thanks for the reviews all. Going to merge to try out this ruff formatting but happy to follow up and update |
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.
nice
@@ -1794,14 +1794,14 @@ def normalize_keyword_aggregation( | |||
|
|||
|
|||
def _make_unique_kwarg_list( | |||
seq: Sequence[tuple[Any, Any]] | |||
seq: Sequence[tuple[Any, Any]], |
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.
this isn't a docstring. did ruff change this?
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.
Yeah I believe from ruff's upgrade to v0.1.13
in this PR
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.
great.
* Bump ruff * Format docstrings using ruff * Fix double {{ examples * Missed shift * Add comma * Remove import * Remove old flake8 validation * Remove unit test * Revert "Remove unit test" This reverts commit 6386676. * Revert "Remove old flake8 validation" This reverts commit d004398. * docstring formatting * Ignore formatting in scripts * Ignore flake8 conflicts with ruff
closes #56804