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

Preserve metaclass when slots=True #155

Merged
merged 5 commits into from Mar 7, 2017

Conversation

Projects
None yet
3 participants
@YevIgn
Contributor

YevIgn commented Feb 27, 2017

Fixes #154

@hynek

This comment has been minimized.

Member

hynek commented Feb 28, 2017

fyi the tests fail on py2/pypy2

@YevIgn

This comment has been minimized.

Contributor

YevIgn commented Mar 1, 2017

Sorry, doubt I can fix the new test, without bringing analog of six.add_metaclass to the code, or using an ugly hack with eval.

@Tinche

This comment has been minimized.

Member

Tinche commented Mar 1, 2017

Hm. Can we bring in six as a test dependency?

@hynek

This comment has been minimized.

Member

hynek commented Mar 1, 2017

I don’t mind. I don't want runtime deps to check for PY3 but reimplementing metaclass magic just for tests seems like a bridge too far.

@codecov

This comment has been minimized.

codecov bot commented Mar 1, 2017

Codecov Report

Merging #155 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #155   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         551    551           
  Branches      122    122           
=====================================
  Hits          551    551
Impacted Files Coverage Δ
src/attr/_make.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c8bd46...39a4299. Read the comment docs.

@hynek hynek requested a review from Tinche Mar 1, 2017

@hynek

This comment has been minimized.

Member

hynek commented Mar 1, 2017

Tin please take this; I have willfully forgotten everything I ever knew about meta classes.

@Tinche

This comment has been minimized.

Member

Tinche commented Mar 1, 2017

Sure thing. I'm sick atm though so it'll have to wait a little

@hynek

This comment has been minimized.

Member

hynek commented Mar 4, 2017

Get well soon!

@hynek hynek added this to the 17.1 milestone Mar 4, 2017

@Tinche

Tinche approved these changes Mar 7, 2017 edited

LGTM. It's a simple change, instead of assuming the metaclass of the class we're changing is type, first fetch the metaclass with the type() builtin, then use that.

The docs are failing because of the logo (Warning, treated as error:

CHANGELOG.rst:None: WARNING: nonlocal image URI found: https://attrs.readthedocs.io/en/latest/_static/attrs_logo.png), so it's unrelated.

@hynek

This comment has been minimized.

Member

hynek commented Mar 7, 2017

I’ve updated the branch and it passes now.

Thanks for the input and I hope you feel better!

@hynek hynek merged commit c70a910 into python-attrs:master Mar 7, 2017

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 2c8bd46
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

hynek added a commit that referenced this pull request Mar 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment