-
Notifications
You must be signed in to change notification settings - Fork 903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix warnings in remaining modules #12406
Changes from all commits
8bb6bf3
9b30293
a81bc54
26c21b6
2ae6103
f2ec687
f31931d
d8f33ec
54ebfa5
f103732
0ece14c
df76783
0e3d164
50129cd
bef347c
69d9166
3fc7abb
cc19d53
f55de12
23c2982
46598be
dc8ba9c
566e3d0
2261411
1e3e801
c0fee9e
567032a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,8 @@ | |
|
||
import cudf | ||
|
||
pytestmark = pytest.mark.filterwarnings("ignore::FutureWarning") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this only ignore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could do that, but I didn't see much value in it. Currently IIRC the list would be cudf, pandas, and numpy (pandas uses a deprecated numpy API somewhere in one example), but in principle it could also come from any other deprecated API that pandas calls and doesn't intercept the warning for, for instance. Given how much of our examples are copied straight from pandas, and since in examples we don't have the flexibility to insert things like |
||
|
||
|
||
def _name_in_all(parent, name): | ||
return name in getattr(parent, "__all__", []) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do type declarations work here? Is the type only declared in one branch of the
if
, or is it assumed that a type definition in one branch of theif
also applies to the otherelse
clause? Is it possible/better to forward-declare the type ofself.dir_
before entering theif
/else
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, types in both branches must be the same. That is how mypy always works. Here's an example (I'm using
sys.argv
rather than a trivialif True
because mypy is smart enough to ignore unreachable branches).results in
Note that in this example I didn't even bother annotating the type, mypy just inferred the types and noticed the incompatibility.
It is possible to forward-declare the type, but it's not something I've seen done much and not anywhere in cudf except for class members. I would be open to discussing that change, but it's out of scope for this PR since only doing it here would be inconsistent with typing everywhere else in cudf (which is already fairly inconsistent at the moment).