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

Why strip leading underscores from attribute names? #1060

Closed
mrolle45 opened this issue Nov 28, 2022 · 3 comments
Closed

Why strip leading underscores from attribute names? #1060

mrolle45 opened this issue Nov 28, 2022 · 3 comments

Comments

@mrolle45
Copy link

mrolle45 commented Nov 28, 2022

This practice leads to errors in cases like:

class C:
    _1: int          # Already mentioned in your docs.
    x: int
    _x: int          # SyntaxError: duplicate argument 'x' in function definition.

Can you explain to me what is the use case for stripping the underscores. The altered name only appears, as far as I can tell, as a parameter name in the generated init method.
EDIT: attrs.evolve is affected, also:

  • In this case:
    @attrs.define
    class C:
        _x: int
        __y: int
    @attrs.define
    class D:
        __x__: int
  • evolve(C(), ...) needs keywords
    • x.
    • C__y. The class def replaces __y with _C__y before it gets to attrs.define. evolve removes the leading underscore from the attribute name.
  • BUG: evolve(D(), ... is impossible with any keywords. The compiler doesn't mangle dunder names, so the attribute is D.__x__. The generated __init__ has parameter x__ because define() stripped both underscores from the parameter name. evolve() only strips the first underscore, and so tries to call `D.init(x_=something).
  • To fix this bug, evolve() should remove all leading underscores. Replace _funcs.py line 366
    init_name = attr_name if attr_name[0] != "_" else attr_name[1:]
    with
    init_name = lstrip(attr_name, "_")

If not, then I consider this a bug and the behavior should be removed. Or deprecated, at least.

If you are going to deprecate it, then I would add a keyword to define() and field(), called (for instance) keep_unders with a default of False. A True value will generate __init__ or __attrs_init__ using the actual attribute name without stripping. Any call to the class constructor will use the unstripped name. After the deprecation period, keep_unders will be ignored and treated as True. This parameter can be deprecated and removed in a later version.

@hynek
Copy link
Member

hynek commented Nov 28, 2022

This is explained in the documentation & will be improved by #950.

@hynek hynek closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2022
@mrolle45
Copy link
Author

I see you've been working on this issue already, as in #950.

It looks like I'll see the improvements in a new release of attrs.

However, note that I added a BUG description to my original comment, and I'd like you to see that it gets fixed.

Thank you.

@asford
Copy link
Contributor

asford commented Nov 30, 2022

I see you've been working on this issue already, as in #950.

It looks like I'll see the improvements in a new release of attrs.

However, note that I added a BUG description to my original comment, and I'd like you to see that it gets fixed.

Thank you.

@mrolle45

I'd like to use this thread, though the issue is already closed, to provide some constructive feedback on why there are a few thumbs down on your comment.

In a project of this type we are all working together to implement an open source library. In this effort we all have important roles. Hynek is operating as maintainer and primary author, but there are other contributors. Here, you're actually playing a very important role as an issue contributor.

One of our roles as issue contributors, or even as contributing authors to attrs, is to make very very efficient use of Hynek's time. Writing effective and clear issue descriptions, with repro cases of the type you've provided is extremely useful. Thanks!

However, it's also very important to look through the context in the project to crosslink your issues, determine if work is already in progress or if the behavior is known. In this issue and the other that you opened, you didn't do this effectively because you didn't evaluate or demonstrate if the referenced PR would address your repro and didn't carry those references forward into the issue.

The best way to see issues fixed in a project is to provide very solid context, ideally even at the level of a test case for the issue that you are seeing. The tone ending your last comment (" I'd like you to see that it gets fixed") was not effective in ensuring your issue will be addressed. As issue reporters, our time is actually less precious than the core maintainer's.

Even from a strictly self-interested perspective, where you only care about having the issue resolved, it would be far more effective to read the context carefully and ask "what can I do help ensure this is fixed?".

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

No branches or pull requests

3 participants