-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
check if pyparsing <<= is broken instead of checking the version #2380
Conversation
bad_pyparsing = f is None | ||
except: | ||
bad_pyparsing = True | ||
if pyparsing.__version__ == '2.0.0' and bad_pyparsing: |
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.
I would take this further and replace the version check below this one with if bad_pyparsing:
.
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.
then one must move the monkey patch up as 1.5.6 does not have the <<= operator?
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.
oh no nevermind.
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.
actually as its monkey patched anyway, why is the version check here in the first place?
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.
I want to avoid monkeypatching if the implementation works. That used to be by a version check, but as you point out, if pyparsing is already patched (without a version number change), that check isn't good enough.
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.
let me go through the versions
1.5.6:
no <<=
-> patch
1.5.7:
broken <<=
-> patch
2.0.0
broken <<=
-> no patch -> abort (?)
2.0.1:
ok
to me it seems the 2.0.0 check can be removed completely and possibly the 2.0.1 version check replaced with a feature check
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.
Yes -- I think the feature check alone is the way to go. And since we are able to patch effectively, the setupext.py can be strictly a version check for 1.5.6 or later.
👍 |
distributions with packaged pyparsing might have already patched pyparsing but not bumped the version number. closes matplotlib#2376
updated to remove the 2.0.0 version check and only monkeypatch if <<= is broken |
check if pyparsing <<= is broken instead of checking the version
f = pyparsing.Forward() | ||
f <<= pyparsing.Literal('a') | ||
bad_pyparsing = f is None | ||
except: |
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.
Do you know which exceptions we might expect? I'd like to discourage bare exception catching...
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.
That's a good point. I think it's a TypeError
indicated "unsupported operand type(s) for ..."
distributions with packaged pyparsing might have already patched
pyparsing but not bumped the version number.
closes #2376