-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: read_excel failing to check older xlrd versions properly #39355
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.
Thanks @lithomas1! Seems to me we should be using _optional._get_version
. Also I think there is one in the tests - can you change that too.
Hello @lithomas1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-01-31 02:25:23 UTC |
@lithomas1 I think we still want to use LooseVersion, can apply it to the result of |
pre-commit failing
|
@lithomas1 can you merge master and update |
Co-authored-by: Simon Hawkins <[email protected]>
@lithomas1 Can also remove |
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.
lgtm - thanks @lithomas1!
thanks @lithomas1 |
@meeseeksdev backport 1.2.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
hmm this is not backporting cleanly, @lithomas1 can you follow the instructions above and put up a PR |
…xlrd versions properly
…ons properly (#39535) Co-authored-by: Thomas Li <[email protected]>
I ran into this bug on pandas-2.1 without xlrd present no problem; but when installed, excel file is not loaded:
|
@brobr - can you raise this as a new issue |
Probably should be in 1.2.1 given that this occurred after the deprecation of the xlrd engine in 1.2.
cc @rhshadrach