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.
| 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, slices and tuples containing slices are treated as | ||
| hashable if hash(obj) does not raise TypeError. |
There was a problem hiding this comment.
i think this extra comment is unnecessary given the description in the parameters section
| # This should never be reached, but let's be explicit about it | ||
| raise ValueError("Too many indices") # pragma: no cover | ||
| if all( | ||
| (is_hashable(x) and not _contains_slice(x)) or com.is_null_slice(x) |
There was a problem hiding this comment.
i think this is the only usage of _contains_slice, so that can be removed?
| | ||
| if is_scalar(key) or ( | ||
| isinstance(labels, MultiIndex) and is_hashable(key) and not contains_slice | ||
| isinstance(labels, MultiIndex) and is_hashable(key, allow_slice=False) |
There was a problem hiding this comment.
i think this changes logic slightly by disallowing slice keys whereas the old code just disallowed tuples-containing-slice?
| def __hash__(self): | ||
| raise TypeError("Not hashable") | ||
| | ||
| class HashableSlice: |
There was a problem hiding this comment.
comment that we can get rid of that once we drop py311?
2 participants
Changes for #55152
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.