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
Remove use of private attributes in smtpd #48434
Comments
Executive summary of the patch: The attached patch removes the use of __private attributes in the smtpd Summary of the patch's changes:
The existing unit tests contained within test_smtplib pass. Additional There is a chance this patch will break backward compatibility with |
Would this patch be acceptable, yes or no? |
The patch as-is can't be accepted if not for Python 4.x maybe, obviously because it's just too breaking. A proper patch would provide aliases for the removed attributes and raise a DeprecationWarning in case they are accessed. Also, I see no reason in turning public attributes like self.__line, self.__state and self.__greeting. Finally, tests should definitively be written. |
Giampaolo, I think I can see where you're coming from: assuming that someone else must have also had to resort to the name-mangling hack to extend the class? In that case yes, my patch would break their code. I'll look at re-working it to use properties while retaining the underlying attributes. Would that be acceptable? What additional tests would you deem necessary? |
I guess it would. Deciding to use that naming convention was a bad design choice in the first place, hence your proposal is perfectly reasonable, in my opinion.
I think that a test which iterates over all renamed attributes and makes sure that DeprecationWarning is raised would be fine. |
With all due respect, this sounds a bit silly. If the attributes were of the "__private" kind, they weren't meant to be used by other classes, and so there's no problem in making them public. (the reverse would be more annoying) However, making them public means they should be maintained with the same semantics in future versions, which might be too much of a burden. In this case, perhaps you want to make them of the "_private" kind instead. |
Generally I would agree with you but this case is different, imho. |
After discussing with core devs at the EuroPython sprint I will implement a different approach: new attributes with the old, private attributes implemented as properties over the new attributes. The properties responsible for this will raise PendingDeprecationWarnings. I'll also be improving (well, *implementing*) test coverage for the module while I'm at it. |
Committed in revision 83125. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: