Skip to content
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

DEPR: remove Index fastpath kwarg #29725

Merged
merged 9 commits into from
Nov 27, 2019
Merged

Conversation

jbrockmendel
Copy link
Member

No description provided.

fastpath=None,
tupleize_cols=True,
**kwargs,
cls, data=None, dtype=None, copy=False, name=None, tupleize_cols=True, **kwargs,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how we historically handle but unfortunate this accepts kwargs - I suppose if someone keeps supplying fastpath nothing will change. Not sure if we should explicitly raise now in that case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed test_deprecated_fastpath so instead of checking for a FutureWarning, it checks for a TypeError

@WillAyd WillAyd added the Deprecate Functionality to remove in pandas label Nov 20, 2019
@WillAyd WillAyd added this to the 1.0 milestone Nov 20, 2019
@jbrockmendel
Copy link
Member Author

@WillAyd are the mypy complaints actually caused by something here or just a coincidence?

@WillAyd
Copy link
Member

WillAyd commented Nov 20, 2019

Doesn't make sense on first glance; just restarted let's see

@@ -269,6 +269,8 @@ or ``matplotlib.Axes.plot``. See :ref:`plotting.formatters` for more.

**Other removals**

- Removed the previously deprecated :meth:`Index.summary` (:issue:`18217`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this was what I had in mind for whatsnew placing.

@jbrockmendel
Copy link
Member Author

@simonjayhawkins any ideas whats going on with mypy here?

@charlesdong1991
Copy link
Member

might be relevant to this issue @jbrockmendel

python/mypy#1174

@simonjayhawkins
Copy link
Member

@simonjayhawkins any ideas whats going on with mypy here?

we had this issue before, see #29205 (comment)

did reach an agreement whether it is a bug or expected behaviour.

the change to the function signatures has triggered this.

@jbrockmendel
Copy link
Member Author

did reach an agreement whether it is a bug or expected behaviour.

suggestions on what to do here? It looks like @TomAugspurger was working on making a fixture for just [Index, Series] at one point

@TomAugspurger
Copy link
Contributor

fixture for just [Index, Series] at one point

Do you mean a TypeVar?

I don't recall the details, but I think my preference was to add type: ignore to the lines in the test files that were causing the (maybe buggy) inference by mypy. But I didn't really understand the issue enough to say what the expected / right behavior is.

@jbrockmendel jbrockmendel changed the title DEPR: remove Index.summary, fastpath kwarg DEPR: remove Index fastpath kwarg Nov 25, 2019
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. I see your typing TODOs, any objections to simply merging and fixing later? @simonjayhawkins

@simonjayhawkins
Copy link
Member

lgtm. I see your typing TODOs, any objections to simply merging and fixing later? @simonjayhawkins

sure

@WillAyd WillAyd merged commit c198f65 into pandas-dev:master Nov 27, 2019
@WillAyd
Copy link
Member

WillAyd commented Nov 27, 2019

Thanks @jbrockmendel

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@jbrockmendel jbrockmendel deleted the depr-index branch April 5, 2020 17:35
@@ -30,6 +31,19 @@ def adjust_negative_zero(zero, expected):
return expected


# TODO: remove this kludge once mypy stops giving false positives here
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonjayhawkins any idea if this is something we can address?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants