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

pass bases keyword argument to make_class #152

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@brainysmurf
Contributor

brainysmurf commented Feb 23, 2017

Currently attr is hard-coded to use (object,) as the bases argument in the type() call. Adding bases as keyword argument would allow attr.make_class to be used in more situations (where you have to use a subclass, and you have to make a class dynamically).

Testing-wise, not sure if another test is needed for this well-understood feature; I ran pytest with this modification and it passed all of them.

brainysmurf added some commits Feb 21, 2017

Update _make.py
Allow make_class to subclass by using bases keyword argument. It would be useful to be able to do this when writing a framework with attrs.
@codecov

This comment has been minimized.

codecov bot commented Feb 23, 2017

Codecov Report

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

@@          Coverage Diff          @@
##           master   #152   +/-   ##
=====================================
  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 f232cc8...974cc7c. Read the comment docs.

@hynek

This comment has been minimized.

Member

hynek commented Feb 23, 2017

Wellll, testing-wise you’ve lowered the code coverage. :)

Before you spend more time coding: could you provide us with a practical example of what you want to achieve? That’s also something that should go into examples.rst if we admit this change.

@brainysmurf

This comment has been minimized.

Contributor

brainysmurf commented Feb 24, 2017

I coded up an example. I'd like to build a model where equality is defined by the attributes and properties being equal (equal key with equal values). This works to the specifications:

import attr

class Base:
    def __eq__(self, other):
        defaults = dir(attr.make_class('None', {}))
        keys = []
        for attr_or_prop in [key for key in dir(self) if key not in defaults]:
            keys.append(attr_or_prop)
        return all([getattr(self, key) == getattr(other, key) for key in keys])

@attr.s(cmp=False)
class ModelOne(Base):
    stuff = attr.ib(default='')

@attr.s(cmp=False)
class ModelSame(Base):
    stuff = attr.ib(default='')

@attr.s(cmp=False)
class ModelDifferent(Base):
    stuff = attr.ib(default='different!')

one = ModelOne()
same = ModelSame()
different = ModelDifferent()

assert one == same
assert one != different
assert same != different

Now I'd like to do the same behavior by making the class dynamically with attr.make_class (I have good reasons to do this, I'm actually writing a class decorator and building properties on it):

attributes = {
    'stuff': attr.ib('')
}

# cannot use subclass, what do do?
one = attr.make_class('One', attributes)
same = attr.make_class('Same', attributes)

assert one == same  # AssertionError

By passing bases I can do this, but is there a cleaner way of doing this?

@hynek

This comment has been minimized.

Member

hynek commented Feb 25, 2017

Right.

So the new functionality has to be tested to make sure it works and doesn’t break in the futures. Simply write a test that creates a class that uses a custom subclass and you’re golden. I would assume it also can be simplified by pulling the (object,) into the signature as a default argument.

Also you’re gonna want everyone to know about that new feature so you need to

  1. add the argument to the docstring
  2. add the change to the changelog.

Feel free to check out https://github.com/python-attrs/attrs/blob/master/CONTRIBUTING.rst if you’re unsure how to do that!

Thanks!

@brainysmurf

This comment has been minimized.

Contributor

brainysmurf commented Feb 25, 2017

As far as using (object,) for the default argument, I got into the habit of always using None, but the point is actually to use immutable objects, which a tuple is.

@brainysmurf

This comment has been minimized.

Contributor

brainysmurf commented Feb 26, 2017

Submitted for your approval :)

@hynek hynek closed this in d86e28b Mar 4, 2017

@hynek

This comment has been minimized.

Member

hynek commented Mar 4, 2017

Thank you for your contribution! I’ve added some minor fixes I didn’t want to boss you around for. :)

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