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.
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.
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.
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.
prob should use our fixture for these (future PR)
doc-warning |
git diff upstream/master -u -- "*.py" | flake8 --diff