CLN: Dead version checking code post minimum version bump by mroeschke · Pull Request #27063 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation9 Commits9 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- xref Bump minimum verions #26832
- tests added / passed
- passes
git diff upstream/master -u -- "*.py" | flake8 --diff
Matt Roeschke added 3 commits
| 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'] |
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.
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.
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?
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> .
Matt Roeschke added 2 commits
lgtm, checks are failing, ping on green.
| else: |
|---|
| timezones = ['UTC', 'Asia/Tokyo', 'US/Eastern', |
| 'dateutil/America/Los_Angeles'] |
| timezones = ['UTC', 'Asia/Tokyo', 'US/Eastern', |
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)
home/vsts/work/1/s/doc/source/whatsnew/v0.25.0.rst:828: WARNING: Unknown interpreted text role "method".
doc-warning
