Skip to content

Remove redundant setters #2747

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

Closed
wants to merge 2 commits into from
Closed

Remove redundant setters #2747

wants to merge 2 commits into from

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