Skip to content
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

Merged
merged 1 commit into from Sep 4, 2013

Conversation

juliantaylor
Copy link
Contributor

distributions with packaged pyparsing might have already patched
pyparsing but not bumped the version number.
closes #2376

bad_pyparsing = f is None
except:
bad_pyparsing = True
if pyparsing.__version__ == '2.0.0' and bad_pyparsing:
Copy link
Member

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:.

astropy/astropy#1172

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh no nevermind.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@mdboom
Copy link
Member

mdboom commented Sep 4, 2013

👍

distributions with packaged pyparsing might have already patched
pyparsing but not bumped the version number.
closes matplotlib#2376
@juliantaylor
Copy link
Contributor Author

updated to remove the 2.0.0 version check and only monkeypatch if <<= is broken

mdboom added a commit that referenced this pull request Sep 4, 2013
check if pyparsing <<= is broken instead of checking the version
@mdboom mdboom merged commit 5f8624d into matplotlib:v1.3.x Sep 4, 2013
f = pyparsing.Forward()
f <<= pyparsing.Literal('a')
bad_pyparsing = f is None
except:
Copy link
Member

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...

Copy link
Member

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 ..."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants