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

Remove use of private attributes in smtpd #48434

Closed
richard mannequin opened this issue Oct 23, 2008 · 9 comments
Closed

Remove use of private attributes in smtpd #48434

richard mannequin opened this issue Oct 23, 2008 · 9 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@richard
Copy link
Mannequin

richard mannequin commented Oct 23, 2008

BPO 4184
Nosy @pitrou, @giampaolo, @dhellmann
Files
  • smtpd.py-patch: Patch to replace use of private attributes in smtpd.py
  • 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:

    assignee = None
    closed_at = <Date 2010-07-24.09:53:11.465>
    created_at = <Date 2008-10-23.01:17:13.379>
    labels = ['type-feature', 'library']
    title = 'Remove use of private attributes in smtpd'
    updated_at = <Date 2010-07-24.09:53:11.463>
    user = 'https://bugs.python.org/richard'

    bugs.python.org fields:

    activity = <Date 2010-07-24.09:53:11.463>
    actor = 'richard'
    assignee = 'richard'
    closed = True
    closed_date = <Date 2010-07-24.09:53:11.465>
    closer = 'richard'
    components = ['Library (Lib)']
    creation = <Date 2008-10-23.01:17:13.379>
    creator = 'richard'
    dependencies = []
    files = ['11867']
    hgrepos = []
    issue_num = 4184
    keywords = []
    message_count = 9.0
    messages = ['75132', '109573', '109582', '109619', '109628', '109634', '109664', '111264', '111436']
    nosy_count = 5.0
    nosy_names = ['richard', 'pitrou', 'giampaolo.rodola', 'doughellmann', 'BreamoreBoy']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue4184'
    versions = ['Python 3.2']

    @richard
    Copy link
    Mannequin Author

    richard mannequin commented Oct 23, 2008

    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.

    @richard richard mannequin added the stdlib Python modules in the Lib dir label Oct 23, 2008
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 8, 2010

    Would this patch be acceptable, yes or no?

    @giampaolo
    Copy link
    Contributor

    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.

    @richard
    Copy link
    Mannequin Author

    richard mannequin commented Jul 8, 2010

    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?

    @giampaolo
    Copy link
    Contributor

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 8, 2010

    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.

    @giampaolo
    Copy link
    Contributor

    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 bpo-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.

    @richard
    Copy link
    Mannequin Author

    richard mannequin commented Jul 23, 2010

    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.

    @richard
    Copy link
    Mannequin Author

    richard mannequin commented Jul 24, 2010

    Committed in revision 83125.

    @richard richard mannequin closed this as completed Jul 24, 2010
    @richard richard mannequin self-assigned this Jul 24, 2010
    @richard richard mannequin added the type-feature A feature request or enhancement label Jul 24, 2010
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants