classification
Title: Remove use of private attributes in smtpd
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: richard Nosy List: BreamoreBoy, doughellmann, giampaolo.rodola, pitrou, richard
Priority: normal Keywords:

Created on 2008-10-23 01:17 by richard, last changed 2010-07-24 09:53 by richard. This issue is now closed.

Files
File name Uploaded Description Edit
smtpd.py-patch richard, 2008-10-23 01:17 Patch to replace use of private attributes in smtpd.py
Messages (9)
msg75132 - (view) Author: Richard Jones (richard) * (Python committer) Date: 2008-10-23 01:17
Executive summary of the patch:

The attached patch removes the use of __private attributes in the smtpd
module allowing it to be extensible without needing to use the
"_<classname>__<attributename>" hack.


Summary of the patch's changes:

1. removes the unused __conn and __addr attributes
2. renames __server to smtp_server
3. renames __lines to received_lines
4. renames __state to smtp_state
5. renames __greeting to seen_greeting, and alters the default to empty
string to match the anticipated data
6. renames __mailfrom to mailfrom
7. renames __date to received_data
8. renames __fqdn to fqdn
9. removes __peer and uses base class' addr attribute

The existing unit tests contained within test_smtplib pass. Additional
tests could be written if it's deemed necessary.

There is a chance this patch will break backward compatibility with
programs that use the private-variable-access hack. A more complex patch
could be written providing greater compatibility if it's deemed necessary.
msg109573 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-08 19:20
Would this patch be acceptable, yes or no?
msg109582 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-07-08 19:47
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.
It would be suitable for the the next major release (3.2 at the current time) and the total removal of the deprecated attributes would happen only in 3.3 if not later.

Also, I see no reason in turning public attributes like self.__line,  self.__state and self.__greeting.
They should just be private (one underscore) and their name kept the same (e.g. no __server -> smtp_server, __greeting -> seen_greting, etc...).

Finally, tests should definitively be written.
msg109619 - (view) Author: Richard Jones (richard) * (Python committer) Date: 2010-07-08 22:12
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?
msg109628 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-07-08 22:31
> Would that be acceptable?

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.

> What additional tests would you deem necessary?

I think that a test which iterates over all renamed attributes and makes sure that DeprecationWarning is raised would be fine.
Also, "__attrname" and "attrname" should be checked for equality.
msg109634 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-07-08 22:44
> The patch as-is can't be accepted if not for Python 4.x maybe,
> obviously because it's just too breaking.

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.
msg109664 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-07-09 01:58
> 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.

Generally I would agree with you but this case is different, imho.
The problem here is that those items didn't have to be declared "__private" in the first place. As a consequence of this I think it's reasonable to expect that during the years some code out there might got to rely on them by using name-mangling hack.
After all smtpd module is very old and SMTPHandler class likely to be subclassed because of its basic implementation, and that is why I see some similarities with issue 8483.
I was of the opinion that just removing the __getattr___ ugliness without passing through the usual DeprecationWarning period supposed to be applied in such circumstances would cause no harm, until I realized I was relying on that very functionality in pyftpdlib and I didn't even know it.
msg111264 - (view) Author: Richard Jones (richard) * (Python committer) Date: 2010-07-23 10:23
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.
msg111436 - (view) Author: Richard Jones (richard) * (Python committer) Date: 2010-07-24 09:53
Committed in revision 83125.
History
Date User Action Args
2010-07-24 09:53:11richardsetstatus: open -> closed
type: enhancement
messages: + msg111436

assignee: richard
resolution: fixed
stage: resolved
2010-07-23 10:23:38richardsetmessages: + msg111264
2010-07-09 01:58:46giampaolo.rodolasetmessages: + msg109664
2010-07-08 22:44:20pitrousetnosy: + pitrou
messages: + msg109634
2010-07-08 22:31:33giampaolo.rodolasetmessages: + msg109628
2010-07-08 22:30:14giampaolo.rodolasetmessages: - msg109625
2010-07-08 22:26:33giampaolo.rodolasetmessages: + msg109625
2010-07-08 22:12:57richardsetmessages: + msg109619
2010-07-08 19:48:59giampaolo.rodolasetversions: - Python 3.1, Python 2.7
2010-07-08 19:47:23giampaolo.rodolasetmessages: + msg109582
2010-07-08 19:20:46BreamoreBoysetnosy: + BreamoreBoy

messages: + msg109573
versions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6
2008-10-24 12:15:10doughellmannsetnosy: + doughellmann
2008-10-23 19:30:28giampaolo.rodolasetnosy: + giampaolo.rodola
2008-10-23 01:17:13richardcreate