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

subclass with mandatory attribute cannot be created when the base class has a factory based one #38

Closed
RonnyPfannschmidt opened this issue Apr 11, 2016 · 24 comments · Fixed by #411
Labels

Comments

@RonnyPfannschmidt
Copy link

failing example

import attr

def test_example():

    @attr.s
    class Base(object):
        attr = attr.ib(default=False)

    @attr.s
    class Sub(Base):
        needed = attr.ib()
@hynek
Copy link
Member

hynek commented Apr 11, 2016

This has been reported before and I’m not sure what to do about it. putting mandatory always before optionals? iow reordering?

@RonnyPfannschmidt
Copy link
Author

they can always be given as keyword

the order should stay the same

it should be possible to give them as keyword instead of a positional argument

once inheritance is involved it seems sensible to go towards keywords in any case

basically all mandatory arguments after a optional one can be primary keyword based

@RonnyPfannschmidt
Copy link
Author

btw, i do in fact use them as keywords, but putting in a default of None seems the wrong way

@hynek
Copy link
Member

hynek commented Apr 11, 2016

I kind of agree, but what’s better? having a sentinel and explode on it?

@RonnyPfannschmidt
Copy link
Author

not quite sure what you mean by the sentinel

for the code in the example i posted i think that Sub(needed='foo') should work since all mandatory arguments where given

@hynek
Copy link
Member

hynek commented Apr 11, 2016

what you sketched it results in

def __init__(self, attr=False, needed):
    pass

which is a syntax error.

So we need some default value for needed here.

@RonnyPfannschmidt
Copy link
Author

i see what you mean, in python3 its *, and valid, for python2 there is need for a workaround in form of a sentinel object and exploding then

@Tinche
Copy link
Member

Tinche commented Sep 15, 2016

I accidentally opened up a duplicate of this.

My personal opinion™ is that we should just allow users to mark attributes as init='kwonly'. That will allow users to fix the problem themselves on Python 3 and consider upgrading on Python 2. 😽

Full example:

@attr.s
class A:
    a = attr.ib()
    b = attr.ib(init='kwonly', default=1)

    # __init__ signature is:
    def __init__(self, a, *, b=1):
        pass

@attr.s
class B(A):
    c = attr.ib()

    # __init__ signature is:
    def __init__(self, a, c, *, b=1):
        pass

Also, in the case of the error we have now, a helpful error message can direct users to use kwonly.

@jdmargulici
Copy link

jdmargulici commented Jan 3, 2017

I ran into this problem as well and I have a slightly different take on it that I wanted to submit. A more natural solution to me would be to write something like:

import attr

def test_example():

    @attr.s
    class Base(object):
        attr = attr.ib(default=False)

    @attr.s
    class Sub(Base):
        needed = attr.ib()
        attr = attr.ib(default=False)

At the cost of a very minor repetition of code, this lets the programmer explicitly control the order in which the attributes should appear for representation purposes or when converting to a tuple. However what I wrote obviously doesn't work in the current version of attr: because of the way inheritance is implemented, Sub receives two 'attr' attributes, which happens to break the __init__ method among other things. I do have a bit of beef with that and was both curious about why attributes don't get overridden in subclasses, and willing to do something about it -then I saw this issue and thought that would be a good starting point for my comment.

@traut
Copy link

traut commented Apr 12, 2017

how about this:

  • add optional class-level var that defines the order of arguments (as list of strings?)
  • if some required argument encountered in the child, fail with error, requesting explicit order of attributes specified?
  • users can use required arguments in children but must provide order themselves explicitly

@RonnyPfannschmidt
Copy link
Author

since this is starting to turn into a mirad of dozens and dozens of options i believe this should either not be supported for the sake of simplicity or be supported only by disabling positional constructor arguments in the complete inheritance tree

@hynek
Copy link
Member

hynek commented Apr 12, 2017

I tend to agree. I’m thinking about an option to @attr.s that changes the mode of __init__ from building a proper signature to def __init__(x=NOTHING, y=NOTHING, z=42, a=NOTHING) and so forth. Then it checks at the beginning if there’s a NOTHING in mandatory attributes. It’s slower but only for people who insist to subclass (I would add electro shocks if I could ;)).

@pgruenbacher
Copy link

pgruenbacher commented Jul 5, 2017

Yea I run into this issue a lot when I have some common optional attributes that I want to share across many classes. e.g.

@attr.s()
class SmallOptions(object):
    verbose = attr.ib(default=None)
    other_option = attr.ib(default=None)

@attr.s()
class Main(SmallOptions):
     required = attr.ib()

I'm kinda inspired by go's embedded struct types:
https://www.goinggo.net/2014/05/methods-interfaces-and-embedded-types.html
http://www.hydrogen18.com/blog/golang-embedding.html
which I'd like to replicate that same sort of composability here. Instead of inheriting from SmallOptions, I'd want class Main to 'embed' SmallOptions instance. Maybe it would look something like so.

@attr.s()
class Main(object):
     required = attr.ib()
     __attrs_embedded__ = {'options': SmallOptions}

with some getattr generated functionality. There would be a SmallOptions instance embedded within Main instance that would be proxied, and can be accessed directly by Main().options as well.

@pgruenbacher
Copy link

or like this

@attr.s()
class Main(object):
     required = attr.ib()
     options = attr.embed(SmallOptions)

malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 1, 2017
malinoff added a commit to malinoff/attrs that referenced this issue Nov 2, 2017
asford added a commit to uw-ipd/attrs that referenced this issue Jul 24, 2018
hynek pushed a commit that referenced this issue Aug 11, 2018
* Added support for keyword-only attributes. Closes #106, and closes #38

(Rebases #281)

Co-authored-by: Alex Ford <[email protected]>

* Add `attr.s`-level `kw_only` flag.

Add `kw_only` flag to `attr.s` decorator, indicating that all class
attributes should be keyword-only in __init__.

Minor updates to internal interface of `Attribute` to support
evolution of attributes to `kw_only` in class factory.

Expand examples with `attr.s` level kw_only.

* Add `kw_only` to type stubs.

* Update changelog for rebased PR.

Hear ye, hear ye. A duplicate PR is born.

* Tidy docs from review.

* Tidy code from review.

* Add explicit tests of PY2 kw_only SyntaxError behavior.

* Add `PythonToOldError`, raise for kw_only on PY2.

* `Attribute._evolve` to `Attribute._assoc`.
@aleaverfay
Copy link

This is a closed issue, and I'm definitely out of my depth as I wade into this discussion, but I'm working (with @asford, fwiw) on a project using attrs, and am getting the "No mandatory attributes after..." error message. I'm at the moment stumped as to what the order of attributes is for my class and which ones have and which lack defaults/factory methods. It would be super useful if the exception printed more about the offending attributes. In my case, I want to know:

what is the first attribute that has a default that appears ahead of an attribute which lacks one

right now, the error message only tells me what attribute lacks a default.

@hynek
Copy link
Member

hynek commented Aug 27, 2018

Could you open a feature request please? Comments on closed bugs get inevitably lost.

@ryantuck
Copy link

For anyone stumbling on this issue and still unclear on how to implement a keyword-only attribute, here's the syntax:

@attr.s
class MyClass:
    x = attr.ib(kw_only=True)

@bar
Copy link

bar commented Apr 8, 2019

it is still unclear what the solution is in python 2, which is not yet deprecated

@hynek can you clarify why this issue is closed even though #411 only tackles python 3?

@hynek
Copy link
Member

hynek commented Apr 12, 2019

Because we didn't find a good way to implement it in Python 2 without introducing a forbidding amount of complexity.

While we're committed to maintain Python 2-compatibility as long as it's feasible, we reserve to not implement new features for it if they are either impossible or too painful to implement/maintain.

@bar
Copy link

bar commented Apr 12, 2019

@hynek while I completely agree, I also think it would be a good idea to state exactly that in the documentation, and/or the error

it is very misleading as the ticket is closed, everything is "fixed", py2 is alive... it makes people think they can get away with a workaround in py2, yet this is not true

@Richard1ybb
Copy link

Can anyone tell me what is the recommended way now?

@hynek
Copy link
Member

hynek commented Jun 2, 2019

Either make the subclass kw_only=True or overwrite the attribute with the default in the subclass:

import attr

def test_example():

    @attr.s
    class Base(object):
        a = attr.ib(default=False)

    @attr.s
    class Sub(Base):
        a = attr.ib()
        needed = attr.ib()

@edam
Copy link

edam commented Jan 8, 2020

I think this should be working code:

@attr.s
class Base():
    optional = attr.ib( default = False, kw_only = True )
    _internal = attr.ib( default = None, init = False )
@attr.s
class Subclass( Base ):
    required = attr.ib()

It isn't though, currently, because:

  • _internal (which is kw_only = False) can not follow optional (which is kw_only = True), even though _internal is init = False and wouldn't appear in an __init__() arg list anyway. (Is this a separate bug?)

  • required (which is kw_only = False) can not follow optional (which is kw_only = True). While I understand the design here, I think it is an imposition on developers. At the very least it would be nice to be able to opt-in to being able to this with something like:

@attr.s( group_kw_only_last_in_init = True )
class Subclass:
    required = attr.ib()

(Obviously with a better option name than that. 😉)

@peteroconnor-bc
Copy link

peteroconnor-bc commented Apr 18, 2020

My Python-2-Compatible solution is just

def validate_not_none(obj, attr_obj, val):
    assert val is not None, "Attribute '{}' needs to be defined (not None)".format(attr_obj.name)

@attr.s
class Base():
    optional = attr.ib(default=False, kw_only=True)
    _internal = attr.ib(default=None, init=False)

@attr.s
class Subclass(Base):
    required = attr.ib(default=None, validator=validate_not_none)

s = Subclass()  # <- Will fail validator

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

Successfully merging a pull request may close this issue.