-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Removed **kwds from parse function in read_excel #51218
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
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.
This would need tests, but apart from that this is an API change, not sure if we can do this without a deprecation cycle
In the comment below, I'm assuming that all valid kwargs that can be passed to
Part of this change is raising if a user passes invalid keyword args. I think we can regard this as a bugfix. I guess this change might also mess up someone who is using positional arguments - I'd have to guess that is quite rare but I suppose that aspect is an API change. Would you agree? |
No problem with making the keywords explicit, but would prefer keeping the order, also would rather deprecate the kwargs instead of removing outright, like we did in other places with unused kwargs |
I think the way forward is to emit a FutureWarning if invalid kwargs are in present. I assume @pacificdragon added the keyword arguments in their current locations to agree with the ordering of other methods, but I haven't checked this. We currently can't add new arguments unless they are at the end to retain backwards compatibility. For this, I think there are two ways forward: a. Add keywords to the tail end of the signature If we go with (a), we'll still have different ordering of arguments which isn't great. With (b), we can add arguments in any order after the deprecation is enforced. So I'd lean toward (b) here unless it's preferred that these arguments are allowed to be positional. |
b sounds good, also +1 on adding a FutureWarning and removing in 3.0 |
I would actually be OK with doing this without a deprecation cycle. I think it is a bug for a function to silently accept arguments it does nothing with. We also have a precedent for doing this in the excel space like #34464 |
Thanks, Everyone for taking action on this. @rhshadrach Created PR for the same 51331 |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.The code works fine locally.
Checking CI/CD pipeline now.