CLN: Remove collections.abc classes from pandas.compat #25957
Conversation
pandas/core/arrays/sparse.py
Outdated
| """ | ||
| SparseArray data structure | ||
| """ | ||
| from collections.abc import Mapping |
There was a problem hiding this comment.
If it'd be preferred, I could instead follow the pattern below:
from collections import abc
...
if isinstance(mapper, abc.Mapping):
...
There was a problem hiding this comment.
I don't think we (as maintainers) have much of a preference on this.
My personal MO is that if you find yourself using it multiple times in the codebase, best to import directly into the namespace (fewer characters).
There was a problem hiding this comment.
From #25957 (review):
we get to the point where we are doing from typing import Iterable
@WillAyd : Hmm...are we really sure we want to do that? IMO, the word Iterable is not immediately obvious as a "type" versus the actual Iterable object itself (e.g. from collections.abc)
IMO, if it's possible, I would prefer that we namespace those.
There was a problem hiding this comment.
Yea the convention for imports from typing are called out in MyPy documentation:
https://mypy.readthedocs.io/en/latest/getting_started.html#the-typing-module
There was a problem hiding this comment.
My personal MO is that if you find yourself using it multiple times in the codebase, best to import directly into the namespace (fewer characters).
Agreed, that's my MO too. The only reason I brought it up here is because these names can be a bit ambiguous, so it might be more readable and immediately obvious to see the abc prefix. Aside from the typing conflicts there's also a conflict where we've named a custom class Iterator:
Line 780 in 19626d2
Will leave the PR as-is for now until more people weigh in. I don't have a strong preference as long as we're consistent throughout the codebase. The first thing I'd think upon seeing Iterable would be the collections.abc version but I'm likely biased from not having done much of anything with typing.
There was a problem hiding this comment.
Ah, I see...sigh...it's rather unfortunate that they decided have Iterable both refer to the object as well as the type.
Fair enough, then let's go with what Python says then. However, I'm relatively certainly that we will be writing abc.Iterable many more times than we will be writing Iterable as a type hint (it's going to be more characters, I suspect 🙂)
There was a problem hiding this comment.
Oops, didn't refresh and see the link about the conventions before my last comment; abc prefix it is then.
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
Generally looks good but I would definitely be +1 on doing from collections import abc and using the namespace prefix from there.
The main reasoning for that is that these will conflict with some items defined in the typing module, so when we get to the point where we are doing from typing import Iterable it would clash with what's done here, and I think giving preference to typing is more idiomatic
Codecov Report
@@ Coverage Diff @@
## master #25957 +/- ##
==========================================
+ Coverage 91.82% 91.82% +<.01%
==========================================
Files 175 175
Lines 52581 52568 -13
==========================================
- Hits 48280 48270 -10
+ Misses 4301 4298 -3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25957 +/- ##
==========================================
+ Coverage 91.82% 91.82% +<.01%
==========================================
Files 175 175
Lines 52581 52563 -18
==========================================
- Hits 48280 48266 -14
+ Misses 4301 4297 -4
Continue to review full report at Codecov.
|
|
Okay, now using the |
|
|
||
| try: | ||
| na_value = collections.UserDict() | ||
| na_value = UserDict() |
There was a problem hiding this comment.
this can be changed (separate PR) as well
|
minor comment but can be a followup., |
|
Thanks @jschendel |
4 participants
git diff upstream/master -u -- "*.py" | flake8 --diffRemoves the following from
pandas.compatin favor of direct imports fromcollections.abc:HashableIterableIteratorMappingMutableMappingSequenceSizedSet