-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
CLN: fix mypy errors in pandas\tests\indexes\interval\test_base.py #28926 #28961
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
CLN: fix mypy errors in pandas\tests\indexes\interval\test_base.py #28926 #28961
Conversation
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.
Thanks @srcole for the PR.
to see the errors, you would need to comment out the ignore_errors in |
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.
@srcole generally looking good. however ci failing https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=18915
can you sort imports. see https://dev.pandas.io/docs/development/contributing.html#import-formatting
I tested it locally, and it seems this change also fixes:
So I removed them from |
@srcole can you rebase |
Thanks for catching that @simonjayhawkins. Yup, I guess I missed that when addressing the merge conflict. Anyway, I removed that line. |
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.
@@ -30,7 +31,7 @@ | |||
class Base: | |||
""" base class for index sub-class tests """ | |||
|
|||
_holder = None | |||
_holder = None # type: Optional[Type[Index]] |
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.
Does this need to be Optional
?
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.
IMO we should keep the Optional
otherwise when we use 3.6 type annotations we'll get
pandas\tests\indexes\common.py:34: error: Incompatible types in assignment (expression has type "None", variable has type "Type[Index]")
The Optional should only be excluded where we have added something = None # type: ...
for the purpose of type checking.
In this case _holder
was set to None
before the type annotation was added.
so when we move to 3.6, we will change to
_holder: Optional[Type[Index]] = None
whereas if the initialisation was purely added for the type annotations we would change to just
_holder: Type[Index]
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.
Isn't the latter what we actually want here though? OK to change to 36 annotation now on this PR since 35 CI should be dropped within the next few days
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.
Isn't the latter what we actually want here though?
IMO out of scope here
but to answer your question, in general, with 3.6, we could reduce the number of assert <some_variable> is not None
that are sometimes needed when class attributes are initialised to None
, but we would need to evaluate on a case by case basis.
OK to change to 36 annotation now on this PR since 35 CI should be dropped within the next few days
i'm not sure a good reason to block since this is not guaranteed.
Thanks @srcole ! |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Hi! I based this change on what I saw in #28947. However, when I test this with
mypy pandas/tests/indexes/interval/test_base.py
, I get this error, which I didn't expect:I thought the "Optional" that was added would accept the "None." But, additionally, in my local environment, I also can't see any of the errors listed in issue #28926, so maybe I don't have something set up right.