Fix: EA attribute specifying whether copy=False is ignored #63130
Conversation
|
@jbrockmendel Could you please take a look in this, If i'm not in wrong direction. |
|
@jbrockmendel Please review these changes, whenever you get a chance. |
| data.fillna(data_missing[1], copy=False) | ||
| tm.assert_extension_array_equal(data, data_missing) | ||
| if self._supports_fillna_copy_false: | ||
| # but with copy=False, this raises for EAs that respect the copy keyword |
There was a problem hiding this comment.
i think this comment should be for EAs that dont respect the copy keyword?
There was a problem hiding this comment.
I've added a comment in the else block for the EAs that don't respect the copy keyword.
There was a problem hiding this comment.
ok but isnt the comment here still wrong?
There was a problem hiding this comment.
i thought about removing that comment earlier but wasn’t fully sure since it was written specifically for the if case. You're right, so I’ll remove it.
| result = data.fillna(data_missing[1]) | ||
| assert result[0] == data_missing[1] | ||
| res_copy = data.fillna(fill_value, copy=True) | ||
| tm.assert_extension_array_equal(res_copy, expected) |
There was a problem hiding this comment.
can you keep using data_missing instead of defining a new expected. i.e. minimize the diff
| def test_fillna_no_op_returns_copy(self, data, request): | ||
| super().test_fillna_no_op_returns_copy(data) | ||
|
|
||
| def test_fillna_readonly(self, data_missing): |
There was a problem hiding this comment.
getting rid of this is perfect. i think there are other subclasses that override this too that we also want to get rid of
|
@jbrockmendel I've updated the code as per your suggestion. Please let me know if this also needs a whatsnew entry. |
2 participants
Please let me know if my approach or fix needs any improvements . I’m open to feedback and happy to make changes based on suggestions.
Thank you!