-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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: Dead version checking code post minimum version bump #27063
CLN: Dead version checking code post minimum version bump #27063
Conversation
pandas/io/html.py
Outdated
if LooseVersion(bs4.__version__) <= LooseVersion('4.2.0'): | ||
raise ValueError("A minimum version of BeautifulSoup 4.2.1 " | ||
"is required") | ||
minimum_bs4_version = VERSIONS['bs4'] |
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 bs4 = import_optional_dependency('bs4')
work here? I don't recall why I didn't use it.
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.
So this actually done above:
def _importers():
# import things we need
# but make this done on a first use basis
global _IMPORTS
if _IMPORTS:
return
global _HAS_BS4, _HAS_LXML, _HAS_HTML5LIB
bs4 = import_optional_dependency("bs4", raise_on_missing=False,
on_version="ignore")
_HAS_BS4 = bs4 is not None
But I think you're right; bs4 = import_optional_dependency('bs4')
can be called here again with the raising behavior.
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.
Ah so this wasn't changed because import_optional_dependency
will raise a ImportError
while this path raises a ValueError
(there's a test for this).
I think it's worth changing to an ImportError
. Thoughts @TomAugspurger?
Codecov Report
@@ Coverage Diff @@
## master #27063 +/- ##
===========================================
- Coverage 92.04% 41.86% -50.19%
===========================================
Files 180 180
Lines 50713 50711 -2
===========================================
- Hits 46680 21230 -25450
- Misses 4033 29481 +25448
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #27063 +/- ##
==========================================
- Coverage 92.04% 91.84% -0.21%
==========================================
Files 180 180
Lines 50714 50728 +14
==========================================
- Hits 46680 46590 -90
- Misses 4034 4138 +104
Continue to review full report at Codecov.
|
Agreed with changing to ImportError.
…On Wed, Jun 26, 2019 at 3:49 PM Matthew Roeschke ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pandas/io/html.py
<#27063 (comment)>:
> @@ -831,9 +832,13 @@ def _parser_dispatch(flavor):
raise ImportError(
"BeautifulSoup4 (bs4) not found, please install it")
import bs4
- if LooseVersion(bs4.__version__) <= LooseVersion('4.2.0'):
- raise ValueError("A minimum version of BeautifulSoup 4.2.1 "
- "is required")
+ minimum_bs4_version = VERSIONS['bs4']
Ah so this wasn't changed because import_optional_dependency will raise a
ImportError while this path raises a ValueError (there's a test for this).
I think it's worth changing to an ImportError. Thoughts @TomAugspurger
<https://github.com/TomAugspurger>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27063?email_source=notifications&email_token=AAKAOIXHASI3SAQLUFAQEPLP4PI4HA5CNFSM4H3VJN32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4YO34A#discussion_r297860479>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAKAOIWRYUDQGKUOCF5K3TLP4PI4HANCNFSM4H3VJN3Q>
.
|
lgtm, checks are failing, ping on green. |
else: | ||
timezones = ['UTC', 'Asia/Tokyo', 'US/Eastern', | ||
'dateutil/America/Los_Angeles'] | ||
timezones = ['UTC', 'Asia/Tokyo', 'US/Eastern', |
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.
prob should use our fixture for these (future PR)
doc-warning |
git diff upstream/master -u -- "*.py" | flake8 --diff