Skip to content

Preserve metaclass when slots=True#155

Merged
hynek merged 5 commits intopython-attrs:masterfrom
YevIgn:issue/154
Mar 7, 2017
Merged

Preserve metaclass when slots=True#155
hynek merged 5 commits intopython-attrs:masterfrom
YevIgn:issue/154

Conversation

@YevIgn
Copy link
Copy Markdown
Contributor

@YevIgn YevIgn commented Feb 27, 2017

Fixes #154

@hynek
Copy link
Copy Markdown
Member

hynek commented Feb 28, 2017

fyi the tests fail on py2/pypy2

@YevIgn
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

Tinche commented Mar 1, 2017

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

@hynek
Copy link
Copy Markdown
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
Copy link
Copy Markdown

codecov Bot commented Mar 1, 2017 •
edited
Loading

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 March 1, 2017 11:39
@hynek
Copy link
Copy Markdown
Member

hynek commented Mar 1, 2017

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

@Tinche
Copy link
Copy Markdown
Member

Tinche commented Mar 1, 2017

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

@hynek
Copy link
Copy Markdown
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
View reviewed changes
Copy link
Copy Markdown
Member

@Tinche Tinche left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
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
hynek added a commit that referenced this pull request Mar 7, 2017
@hynek
Add changelog entry for #155
8827b71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tinche Tinche Tinche approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

17.1

Development

Successfully merging this pull request may close these issues.

3 participants

@YevIgn @hynek @Tinche