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

Get rid of properties in builders #929

Open
1 task done
thesadru opened this issue Dec 13, 2021 · 4 comments
Open
1 task done

Get rid of properties in builders #929

thesadru opened this issue Dec 13, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@thesadru
Copy link
Contributor

thesadru commented Dec 13, 2021

Summary

Builders should be remade to not use properties and instead they should just accept direct mutation using their attributes. Setters or properties which are not directly built may remain but they shouldn't be the main way to use the builder.

Why is this needed?

Currently, you are required to use long builders using fairly unpythonic setters instead of just being able to set everything in the __init__. Normally this would've been possible thanks to attrs but since the properties are prefixed with underscores pyright cannot understand that attrs is fine with the underscores being left out.

Ideal implementation

# before
class SomeBuilder:
    _whatever: module.SomeEnum

    _something: int = attr.field(kw_only=True)
    _other: str = attr.field(kw_only=True)

    @property
    def whatever(self) -> module.SomeEnum:
        return self._whatever
    # <repeated for all other fields>

    def set_whatever(self: T, x: Union[int, module.SomeEnum]) -> T:
        self._whatever = module.SomeEnum(x)
        return self
    # <repeated for all other fields>
# after
class SomeBuilder:
    whatever: module.SomeEnum

    something: int = attr.field(kw_only=True)
    other: str = attr.field(kw_only=True)

    def set_whatever(self: T, x: Union[int, module.SomeEnum]) -> T:
        self.whatever = module.SomeEnum(x)
        return self
    # <repeated for all other fields>

Checklist

  • I have searched the issue tracker and have made sure it's not a duplicate. If it is a follow up of another issue, I have specified it.
@thesadru thesadru added the enhancement New feature or request label Dec 13, 2021
@ahnaf-zamil
Copy link
Contributor

Hmm, I agree with this.

@FasterSpeeding
Copy link
Collaborator

Builders should be remade to not use properties and instead they should just accept direct mutation using their attributes.

This would prevent the library from adding validation to setting calls in the future which isn't ideal seeing as the rest flow needs a lot of validation.

Also how the data is stored internally is impl detail, this pattern you're suggesting would make the internal storage a part of the interface which would change the nature of builders such as the button builder.

@FasterSpeeding
Copy link
Collaborator

FasterSpeeding commented Jan 24, 2022

Normally this would've been possible thanks to attrs but since the properties are prefixed with underscores pyright cannot understand that attrs is fine with the underscores being left out.

That's a pyright/attrs issue and not in scope of this project, you shouldn't let a type checker stop you from doing stuff just because its wrong. You can just pass data to the init if you want to avoid any validation or don't want to use setters

@thesadru
Copy link
Contributor Author

thesadru commented Feb 4, 2022

Makes sense. I suppose we should get rid of all the init=False then at least.

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

No branches or pull requests

3 participants