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

Fix some issues with six dependency #240

Merged
merged 4 commits into from Sep 19, 2017
Merged

Fix some issues with six dependency #240

merged 4 commits into from Sep 19, 2017

Conversation

jmoldow
Copy link
Contributor

@jmoldow jmoldow commented Sep 18, 2017

Add a fallback for six.raise_from, which isn't available in
six 1.4.0. It isn't available until six 1.9.0. We could also
have raised the lower bound for the six requirement, but this is
an easy way to allow clients to keep using their existing
versions of six.

Fix support for the latest version of six, 1.11.0. That release
changed the temporary metaclass returned from
with_metaclass(), such that it directly inherits from type,
instead of inheriting from the target metaclass [1]. We depended
on this detail, and the change caused

.. code-block:: python

TypeError('metaclass conflict: ...')

to be raised when defining a class with with_metaclass(). We
fix this by manually selecting the most derived metaclass, and
including it in our temporary metaclass.

Also, __prepare__ is now defined on the temporary metaclass,
in six 1.11.0 [2]. This allows us to skip our own definition of that
method, when using six>=1.11.0.

Fixes #228.

Fixes #239.

[1] benjaminp/six#191
[2] benjaminp/six#178

Add a fallback for `six.raise_from`, which isn't available in
six 1.4.0. It isn't available until six 1.9.0. We could also
have raised the lower bound for the six requirement, but this is
an easy way to allow clients to keep using their existing
versions of six.

Fix support for the latest version of six, 1.11.0. That release
changed the temporary metaclass returned from
`with_metaclass()`, such that it directly inherits from `type`,
instead of inheriting from the target metaclass [1]. We depended
on this detail, and the change caused

.. code-block:: python

    TypeError('metaclass conflict: ...')

to be raised when defining a class with `with_metaclass()`. We
fix this by manually selecting the most derived metaclass, and
including it in our temporary metaclass.

Also, `__prepare__` is now defined on the temporary metaclass,
in six 1.11.0 [2]. This allows us to skip our own definition of that
method, when using six>=1.11.0.

Fixes #228.

Fixes #239.

[1] <benjaminp/six#191>
[2] <benjaminp/six#178>
@boxcla
Copy link

boxcla commented Sep 18, 2017

Verified that @jmoldow has signed the CLA. Thanks for the pull request!

@Jeff-Meadows
Copy link
Contributor

If we're going to reimplement raise_from in case it's not defined in the currently installed version of six, shouldn't we define it correctly (ie using the raise from syntax if we're in PY3)?

@jmoldow
Copy link
Contributor Author

jmoldow commented Sep 18, 2017

@Jeff-Meadows I didn't want to simply redefine it, because then we'd have code that is identical to what appears in six, and I don't want to get into any licensing grey areas.

I think the way it is is okay. In Python 2, there is no difference. In Python 3, the only difference is the __cause__ attribute, which very few clients will look at. Practically, it only really matters for stacktraces.

My personal preference would be:

  1. Keep as-is, or bump the minimum required version of six.
  2. Remove use of raise_from.
  3. Fully redefine raise_from to match six.

I'd be okay with not using raise_from, since it's more of a nice optimization, rather than something that we must do.

But since six 1.9.0 is over two years old, and since we're doing a major version bump, I guess I'd be fine changing this to increase the minimum version, if we don't want to do weird things like redefine a partially-implemented raise_from.

@Jeff-Meadows
Copy link
Contributor

@jmoldow I'd rather not reimplement than partially reimplement. I think bumping the minimum to 1.9.0 is perfectly fine (I'd also be fine bumping to 1.11.0 in the 2.0 branch and removing the additional with_metaclass code)

@jmoldow
Copy link
Contributor Author

jmoldow commented Sep 18, 2017

Bumped the minimum to 1.9.0.

Copy link
Contributor

@Jeff-Meadows Jeff-Meadows left a comment

Choose a reason for hiding this comment

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

Can you also bump the requirement in requirements.txt?

@jmoldow
Copy link
Contributor Author

jmoldow commented Sep 18, 2017

I removed the entries from requirements.txt, since I don't believe there is anything gained from duplicating everything.

@jmoldow jmoldow merged commit e65d34c into master Sep 19, 2017
@jmoldow jmoldow deleted the six_fixes branch September 19, 2017 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants