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

Jump to bottom

Improvements to ipaddress module #72047

Closed
MoritzS mannequin opened this issue Aug 25, 2016 · 13 comments
Closed

Improvements to ipaddress module #72047

MoritzS mannequin opened this issue Aug 25, 2016 · 13 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@MoritzS
Copy link
Mannequin

MoritzS mannequin commented Aug 25, 2016

BPO 27860
Nosy @ncoghlan, @methane, @serhiy-storchaka, @zhangyangyu, @jacktose
PRs
  • bpo-27860: clean up ipaddress #12774
  • bpo-27860: ipaddress: use cached_property #12832
  • bpo-27860: ipaddress: fix Interface missed some attributes #12836
  • [3.7] bpo-27860: ipaddress: fix Interface constructor #14200
    		•
    Files
  • ipaddress-improvement.patch
    		•

    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 2019-06-19.11:48:12.761>
created_at = <Date 2016-08-25.15:14:28.605>
labels = ['3.7', '3.8', 'type-feature', 'library']
title = 'Improvements to ipaddress module'
updated_at = <Date 2019-06-19.11:48:12.760>
user = 'https://bugs.python.org/moritzs'

    bugs.python.org fields:

    activity = <Date 2019-06-19.11:48:12.760>
actor = 'methane'
assignee = 'none'
closed = True
closed_date = <Date 2019-06-19.11:48:12.761>
closer = 'methane'
components = ['Library (Lib)']
creation = <Date 2016-08-25.15:14:28.605>
creator = 'moritzs'
dependencies = []
files = ['44223']
hgrepos = []
issue_num = 27860
keywords = ['patch']
message_count = 13.0
messages = ['273655', '279600', '279607', '339845', '340240', '340259', '340307', '340308', '343349', '343537', '343671', '345983', '346038']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'pmoody', 'methane', 'SilentGhost', 'serhiy.storchaka', 'moritzs', 'xiang.zhang', 'Jacktose']
pr_nums = ['12774', '12832', '12836', '14200']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27860'
versions = ['Python 3.7', 'Python 3.8']
    @MoritzS
    Copy link
    Mannequin Author

    MoritzS mannequin commented Aug 25, 2016

    This patch fixes the following minor issues with the ipaddress module:

    • Removed unused property _BaseV4._valid_mask_octets
    • Removed unused methods _BaseV4._is_valid_netmask() and _BaseV4._is_hostmask()
    • Replaced several calls to superclass constructors by super()

    It also refactors the constructors of IPv4Network, IPv4Interface, IPv6Network, and IPv6Interface.
    They all now use the new method _get_addr_prefix_tuple() to parse the argument. It now matches the following sentence of the documentation of IPv4/6Interface:
    "The meaning of address is as in the constructor of IPv4Network".

    Additionally they now also accept a bytes or an IPv4/6Address object representing a netmask (or a hostmask [only IPv4]) as second element of an address/netmask tuple. This makes it easier to work with C-APIs that provide netmasks only as bytes object.

    This patch does not try to solve bpo-27683.

    @MoritzS MoritzS mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Aug 25, 2016
    @MoritzS
    Copy link
    Mannequin Author

    MoritzS mannequin commented Oct 28, 2016

    Any updates?

    @ncoghlan
    Copy link
    Contributor

    Peter, are you able to take a look at this or indicate you're happy for someone else to take it? (I relinquished my co-maintainer role for the ipaddress module a while back, so you're the only currently listed maintainer)

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Oct 28, 2016
    @methane
    Copy link
    Member

    methane commented Apr 10, 2019

    @MoritzS Would you create a pull request on GitHub?
    Or may I create a pull request for your patch?

    @methane
    Copy link
    Member

    methane commented Apr 15, 2019

    New changeset 2430d53 by Inada Naoki in branch 'master':
    bpo-27860: use cached_property (GH-12832)
    2430d53

    @methane
    Copy link
    Member

    methane commented Apr 15, 2019

    I am not owner of ipaddress module, so I don't know we should
    support more form of masks.

    On the other hand, IPv4Interface and IPv6Interface expose
    netmask and hostmask attributes when address is not bytes or int.

    These attributes are not documented. I assume it was added
    accidentally, when coping some code from Network classes.

    But there are test for these attributes.
    NetmaskTestMixin_v4.test_valid_netmask runs for IPv4Network and IPv4Interface.
    It checks these attributes when constructor argument is string.

    cpython/Lib/test/test_ipaddress.py

    Lines 422 to 440 in 3c7931e

    def test_valid_netmask(self):
    self.assertEqual(str(self.factory('192.0.2.0/255.255.255.0')),
    '192.0.2.0/24')
    for i in range(0, 33):
    # Generate and re-parse the CIDR format (trivial).
    net_str = '0.0.0.0/%d' % i
    net = self.factory(net_str)
    self.assertEqual(str(net), net_str)
    # Generate and re-parse the expanded netmask.
    self.assertEqual(
    str(self.factory('0.0.0.0/%s' % net.netmask)), net_str)
    # Zero prefix is treated as decimal.
    self.assertEqual(str(self.factory('0.0.0.0/0%d' % i)), net_str)
    # Generate and re-parse the expanded hostmask. The ambiguous
    # cases (/0 and /32) are treated as netmasks.
    if i in (32, 0):
    net_str = '0.0.0.0/%d' % (32 - i)
    self.assertEqual(
    str(self.factory('0.0.0.0/%s' % net.hostmask)), net_str)

    For safety, I added these attributes always, instead of remove them.

    PR-12836 is fixing it, and extracts methods for Network/Interface constructor
    (a part of PR-12774). It doesn't add additional mask form support.

    @methane methane added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Apr 15, 2019
    @methane
    Copy link
    Member

    methane commented Apr 15, 2019

    New changeset 6fa84bd by Inada Naoki in branch 'master':
    bpo-27860: ipaddress: fix Interface missed some attributes (GH-12836)
    6fa84bd

    @methane
    Copy link
    Member

    methane commented Apr 15, 2019

    I merged all cleanups.

    I don't merge accepting any IP representations as mask, because I'm not expert of this module.
    At least, I don't want to pass prefix/netmask by something like IPv4Network("0.0.255.255").

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented May 24, 2019

    I was wondering why it was decided against backporting to 3.7? 6fa84bd fixes an actual bug bpo-35990 (string mask in tuple argument for IPv4Interfaces). Incidentally, there are also no tests for this behaviour.

    Just to note: both merged PRs lacked news entry.

    @SilentGhost SilentGhost mannequin added the 3.7 (EOL) end of life label May 24, 2019
    @methane
    Copy link
    Member

    methane commented May 26, 2019

    I was wondering why it was decided against backporting to 3.7?

    Because I treats this is just a code cleanup.

    6fa84bd fixes an actual bug bpo-35990 (string mask in tuple argument for IPv4Interfaces).

    I didn't thought it was a bug.
    I don't know it is real bug which should be backported to 3.7 for now.

    Incidentally, there are also no tests for this behavior.

    Because I thought there are no change about public (documented) behavior.

    Just to note: both merged PRs lacked news entry.

    NEWS entry is not needed for code cleanup.

    @jacktose
    Copy link
    Mannequin

    jacktose mannequin commented May 27, 2019

    I for one have encountered bpo-35990 in my work and would appreciate a backport.

    @methane
    Copy link
    Member

    methane commented Jun 18, 2019

    OK, I thought I improved only undocumented behavior, but it was documented and previous behavior didn't follow the document.
    I'll backport PR 12836.

    @methane
    Copy link
    Member

    methane commented Jun 19, 2019

    New changeset f532fe5 by Inada Naoki in branch '3.7':
    bpo-27860: ipaddress: fix Interface constructor (GH-14200)
    f532fe5

    @methane methane closed this as completed Jun 19, 2019
    @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
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @methane @ncoghlan @serhiy-storchaka