Skip to content

Conversation

harshil21
Copy link
Member

@harshil21 harshil21 commented Oct 25, 2021

Removes redundant setters since trying to assign on a setter already raises an AttributeError with a similar message we provide.

We can also remove the corresponding tests for this since they are implied. (Edit: removed them)

@harshil21 harshil21 added this to the v14 milestone Oct 25, 2021
@harshil21 harshil21 added the 📋 pending-review work status: pending-review label Oct 25, 2021
@Bibo-Joshi
Copy link
Member

Well, the "similar message" is just can't set attribute and I'd argue that the current messages a quite a bit more informative 😄
Still, I'm not entirely opposed to removing them. the error message for defauls is also simply wrong though since #2363 (even though I wouldn't want to make Defaults mutable), so it should at least be changed.

I vote -0 on removing the setters. @Poolitzer what do you think?

In any case, I think we should keep the tests to make sure that the attributes stay read-only. We'd just have to adapt them abit …

@Poolitzer
Copy link
Member

Im always for more descriptive error messages.

@harshil21
Copy link
Member Author

Closing, then.

@harshil21 harshil21 closed this Oct 26, 2021
@harshil21 harshil21 deleted the rm-redundant-property branch October 26, 2021 15:09
@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📋 pending-review work status: pending-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants