Add PD002 use-of-inplace-argument documentation #2799
Conversation
| @@ -8,6 +8,26 @@ use crate::rules::pandas_vet::fixes::fix_inplace_argument; | |||
| use crate::violation::AlwaysAutofixableViolation; | |||
|
|
|||
| define_violation!( | |||
| ///## What it does | |||
| ///Checks for `inplace=True` inside `pandas` code. | |||
There was a problem hiding this comment.
nit: "pandas code" could be misunderstood to refer to the code of the pandas library.
There was a problem hiding this comment.
I think this section should mention which methods are checked for this parameter.
There was a problem hiding this comment.
I think this section should mention which methods are checked for this parameter.
I'm not sure how to do this exactly. I think it's a bunch of methods.
There was a problem hiding this comment.
Ah ok ... in that case I think it's fine to just list a few of these methods as examples and say that there are more. Ideally the mentions of the example methods would be linked to their pandas documentation.
There was a problem hiding this comment.
I think I've addressed everything here except maybe for more detail about exactly which pandas methods this applies to and links to them maybe? I'm not sure how to do that in kind of a "principled" way, other than just linking to a few random pandas methods that happen to have this option.
If you are okay with it, I'm inclined to leave it as is and see if anyone else has a clear path to making it clearer. I think this is considerably better than no documentation.
There was a problem hiding this comment.
Fair enough linking several would indeed be quite random but I think linking just one as an example would be nice so that you can see how pandas documents the inplace parameter ... since the pandas documentation may change. How about the following?
Several methods from the pandas library accept an `inplace` parameter
(for example the [`pandas.DataFrame.replace`] method). This rule checks
for `inplace=True` in code using the `pandas` library.
// ... rest of documentation ...
[`pandas.DataFrame.replace`]: https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.replace.html
| /// - It removes the ability to use the method chaining style for `pandas` | ||
| /// code. |
There was a problem hiding this comment.
I should have clarified ... I care in particular about the ruff rule output not being longer than 80 chars.
Since the /// is stripped from that the actual lines here can in fact be 88 chars long, so I think code. can be still on the previous line.
| /// | ||
| /// ## Why is this bad? | ||
| /// - `inplace=True` often does not provide a performance benefit. It is | ||
| /// likely to copy dataframes in the background. |
There was a problem hiding this comment.
I think Markdown lists look a bit better if the wrapped text is indented as much as the first line of the list item.
| /// Checks for `inplace=True` in code using the `pandas` library. | ||
| /// | ||
| /// ## Why is this bad? | ||
| /// - `inplace=True` often does not provide a performance benefit. It is |
There was a problem hiding this comment.
nit: In the other rule documentations we have been using * as a list marker rather than - ... it would be nice to be consistent.
#2646