CLN: Add allow_slice to is_hashable function #62567
Conversation
LirongMa
changed the title
|
Are there non-test places where this is needed? |
not so far. Because in your PR(#53743 (comment)) you added the checks along with the call to is_hashable(). I added this 'allow_slice' argument based on Richard's suggestion (#55152 (comment)) Let me make some changes to some of the callers and set 'allow_slice' to be False. |
pandas/io/excel/_xlsxwriter.py
Outdated
| ) | ||
|
|
||
| try: | ||
| self._book = Workbook(self._handles.handle, **engine_kwargs) # type: ignore[arg-type] |
There was a problem hiding this comment.
mypy failed on this comment.
There was a problem hiding this comment.
@jbrockmendel I made changes based on your feedback. Please review.
pandas/core/dtypes/inference.py
Outdated
| If `allow_slice` is False, objects that are slices or tuples containing slices | ||
| will always return False, even if hash(obj) would succeed. | ||
| If `allow_slice` is True or None, slices and tuples containing slices are treated as |
There was a problem hiding this comment.
None is a default value. So for exsiting callers, I don't need to set allow_slice. Unless you want me to change all the callers of is_hahsable and set the allow_slice to be True (if they don't have extra checks for slice). Please let me know.
There was a problem hiding this comment.
just set the default to True.
There was a problem hiding this comment.
Yes, set the default to be True now.
pandas/core/dtypes/inference.py
Outdated
| # Reconsider this decision once this numpy bug is fixed: | ||
| # https://github.com/numpy/numpy/issues/5562 | ||
|
|
||
| def _contains_slice(x: object) -> bool: |
There was a problem hiding this comment.
this is only called once? why does it need to be a function?
There was a problem hiding this comment.
It's to make this line more clean:
if allow_slice is False and _contains_slice(obj):
Otherwise, I have to do:
if allow_slice is False:
if isinstance(x, tuple) and any(isinstance(v, slice) for v in x):
return False
elif isinstance(x, slice):
return False
There was a problem hiding this comment.
I removed _contains_slice.
2 participants
Changes for #55152
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.