bpo-34788: Add support for scoped IPv6 addresses

[opavlyuk]

https://bugs.python.org/issue34788

Automerge-Triggered-By: @asvetlov

[aeros]

@opavlyuk Welcome and thanks for the contribution!

I would advise breaking up this PR into multiple smaller ones (approximately the size of the commits or smaller, but as self-contained chunks starting with the least dependent ones). This would allow for both easier review and debugging of the integration tests. Otherwise, this process will likely take significantly longer.

Also from the perspective of readability, I would recommend replacing the longer code blocks of repeating assertBadSplit() with iteration.

For example on lines 359-368, instead of using this:

        assertBadSplit("3ffe::1::1%scope")
        assertBadSplit("1::2::3::4:5%scope")
        assertBadSplit("2001::db:::1%scope")
        assertBadSplit("3ffe::1::%scope")
        assertBadSplit("::3ffe::1%scope")
        assertBadSplit(":3ffe::1::1%scope")
        assertBadSplit("3ffe::1::1:%scope")
        assertBadSplit(":3ffe::1::1:%scope")
        assertBadSplit(":::%scope")
        assertBadSplit('2001:db8:::1%scope')

I would recommend something like this:

# since this is just an example, I didn't include all of them
bad_split_asserts = ["3ffe::1::1%scope", "1::2::3::4:5%scope", "2001::db:::1%scope",]
for assertion in bad_split_asserts:
    assertBadSplit(assertion)

This would significantly improve the overall readability of the code and more quickly conveys the message of what is being done. This doesn't have to be done when using a similar format for 2-3 lines, but it starts to add an unnecessary number of extra lines and characters when it's done 5+ times.

Edit: Also I forgot to mention that we generally use underscores to represent spaces, not camelCase. So assertBadSplit() should be assert_bad_split().

[opavlyuk]

@aeros167 Thanks for your review!

I would advise breaking up this PR into multiple smaller ones (approximately the size of the commits or smaller, but as self-contained chunks starting with the least dependent ones).

As for me, this PR could not be broken into multiple smaller ones, because it refers to and covers a single issue. Also, all changes depend on each other and could not be merged separately (e.g. tests after main code or vice versa). According to devguide, tests and documentation should be included to PR. So, seems it would not be right to make separate PR`s.
May be, some tests could be considered redundant, but devguide insists on writing tests both for normal behaviour and for error conditions.

Also from the perspective of readability, I would recommend replacing the longer code blocks of repeating assertBadSplit() with iteration.

Please, take a look at

cpython/Lib/test/test_ipaddress.py

Lines 348 to 357 in fb99b6b

	assertBadSplit("3ffe::1::1")
	assertBadSplit("1::2::3::4:5")
	assertBadSplit("2001::db:::1")
	assertBadSplit("3ffe::1::")
	assertBadSplit("::3ffe::1")
	assertBadSplit(":3ffe::1::1")
	assertBadSplit("3ffe::1::1:")
	assertBadSplit(":3ffe::1::1:")
	assertBadSplit(":::")
	assertBadSplit('2001:db8:::1')

In my PR I only extend this test with cases, consisting scope IPv6 addresses. It was already written in such a style. According to devguide, it isn't recommended to make cosmetic changes to unrelated code. PEP8 also tells that, if an existing library has a different style, internal consistency is preferred. So, to keep existing style of test, I haven't put assertions in loop. But, if it is a must, I will fix that lines, of course.

Edit: Also I forgot to mention that we generally use underscores to represent spaces, not camelCase. So assertBadSplit() should be assert_bad_split().

Again,

cpython/Lib/test/test_ipaddress.py

Line 343 in fb99b6b

def assertBadSplit(addr):

was written before me, so I consider not to fix naming style. Also, as far as unitest library is a port of JUnit, it inherits Java naming style in assertion methods. Just take a look at all other assertion method names, they are camelCased. In my opinion, it would be right to keep existing code style in the module.

[aeros]

@opavlyuk

As for me, this PR could not be broken into multiple smaller ones, because it refers to and covers a single issue. Also, all changes depend on each other and could not be merged separately (e.g. tests after main code or vice versa). According to devguide, tests and documentation should be included to PR. So, seems it would not be right to make separate PR`s.
May be, some tests could be considered redundant, but devguide insists on writing tests both for normal behaviour and for error conditions.

Usually, for more involved issues, a single PR represents a self-contained step towards the resolution. Self-contained meaning that the PR also contains the respective documentation changes, test coverage, and is compatible with the existing code-base by itself. This doesn't mean that the entire issue has to be covered at once.

As for this PR specifically, I'm not entirely certain into what self-contained pieces it could be further fragmented into, it's just a general guideline. If it's not reasonably possible to do this, it's not at all required. The most substantial benefit of fragmenting it into multiple PRs is easier debugging when the integration tests aren't passing.

In my PR I only extend this test with cases, consisting scope IPv6 addresses. It was already written in such a style. According to devguide, it isn't recommended to make cosmetic changes to unrelated code. PEP8 also tells that, if an existing library has a different style, internal consistency is preferred. So, to keep existing style of test, I haven't put assertions in loop.

Upon reconsideration and further inspection of the existing tests, keeping the current style of the library is understandable, especially given the sheer volume. Perhaps I'll work on refactoring it in a future PR, but it'd likely have to be done all at once for consistency. When I had first submitted my initial review, I had underestimated the size of the existing tests. Improving the readability would likely be a project on it's own. I wouldn't expect you to refactor all of the entire existing tests within the same issue.

The above also applies to the naming conventions. I'm far more used to our more recent unit tests using the underscore naming. My apologies, it looks like I focused a bit too much on your additions rather than the context.

But, if it is a must, I will fix that lines, of course.

As someone who isn't a core dev, my reviews are just recommendations based on my experiences thus far working within the Python repositories. So if you have reasons for doing anything a particular way and the core devs agree with your justification, you can definitely keep it as it is.

[opavlyuk]

@aeros167, I've added entry to Misc/NEWS.d and resolved issues with suspicious lines in documentation, which caused checks to fail. Now, all checks are passing.
Looking forward to make all necessary fixes and have an approved PR .

[aeros]

The documentation and changes to ipaddress look good as far as I can tell. Next, we just have give the core devs some time to review the PR. Based on the experts index pmoody is the expert for ipaddress, but there is not a github account listed on their issue tracker profile. As a result, I won't be able to mention them in the PR's comment. They're already on the issue's nosy list though.

As a minor fix, the news entry column width is supposed to be 80 characters according to the devguide, so I split it into two lines. Also, I trimmed the path to ipaddress down by replacing it with reST inline markup (mod is an abbreviation of module).

To simply a conditional, I made a minor suggestion for line 1863 of ipaddress.py.

[terryjreedy]

The hundreds of commits listed above is an error that might be fixed by a simple update. But the file conflict has to be fixed first. I will try to do so. The conflict shows why What's New entries should not be added until a PR is merge-ready.

[opavlyuk]

@terryjreedy Sorry, previous reviewers strongly recommended adding that What'sNew (#13772 (comment), #13772 (comment)), so I followed their advice.
Should I keep it, or remove until PR is merge-ready?

[ethanfurman]

@terryjreedy, I thought the point of the new blurb tool was to avoid What's New troubles -- am I missing something?

[terryjreedy]

@lisroach Lisa, you approved the idea on the tracker, and are the only core dev to give an opinion. Can you review at least part of the patch? Do you know ipaddress well enough to merge?

[terryjreedy]

@ethanfurman Blurb solved conflict problems with the single news items file, Misc/NEWS.rst, linked to after html-ified, as changelog from What's New. The single whatsnew/3.9.rst has the same issue as having a single NEWS.rst did. Traditionally, entries were not added with commits, but were selectively added later by a few editors trolling the news file. Adding as we go, with changes in PRs that sit around for months, recreates the old problem.

[terryjreedy]

@opavlyuk There are 3 issues more important at the moment than the whatsnew item.

1. CLA: bot initially said 'signed', then revoked that without explanation, more that a day ago, though I no longer see the initial revocation notice.

2. Commit #: header says you want 'to merge 294 commits', which is wrong. We usually close PRs opened like this and ask for a resubmission. But I believe mistake came later, apparently after initial reviews, which we do not want to close, perhaps with an unneeded and bad force push. (Github is not showing the history of your commits here like it usually does.) I hoped that update merges would fix this (as it did on another issue), but no.

3. Open change request: click [+-] box after gnprice to find them again.

Please ask on core-mentorship list how to deal with 1 and 2 and anything about 3 that you are not sure of.

[opavlyuk]

@terryjreedy , thank you for your help with this PR, I really appreciate your efforts.
About issues:

1. Fixed while resolving 2-nd issue;
2. I figured out, that extra commits appeared after I merged master branch from cpython repo directly to current issue branch, instead of merge it first to master branch of my repo and futher merge to issue branch. Fixed it with force push.
3. As for now, I have a lack of time to write expalnations why I haven't listened to every gnprice's advice, but I`ve already applied all suggestions, that I consider reasonable.

[terryjreedy]

@gnprice Of your many suggestions, opavlyuk has "applied all suggestions, that I consider reasonable". Of the rest, are there any you consider critical, or would you be comfortable merging as is?

[asvetlov]

Looks good and ready to merge.
Please drop a few lines from whatsnew.

Sorry, for the long review, I was pretty busy.
Thanks for your patience.

[asvetlov]

Please drop these lines, they are added by accident.
Victor decided to move this text to another place.

[opavlyuk]

Thanks a lot! Fixed and also corrected typo in my own surname (what a shame).

[miss-islington]

@opavlyuk: Status check is done, and it's a failure ❌ .

[miss-islington]

@opavlyuk: Status check is done, and it's a success ✅ .

[asvetlov]

Thanks!

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 macOS 3.x has failed when building commit 21da76d.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/275/builds/286) and take a look at the build logs.
4. Check if the failure is related to this commit (21da76d) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/275/builds/286

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 25, done.        
remote: Counting objects:   4% (1/25)        
remote: Counting objects:   8% (2/25)        
remote: Counting objects:  12% (3/25)        
remote: Counting objects:  16% (4/25)        
remote: Counting objects:  20% (5/25)        
remote: Counting objects:  24% (6/25)        
remote: Counting objects:  28% (7/25)        
remote: Counting objects:  32% (8/25)        
remote: Counting objects:  36% (9/25)        
remote: Counting objects:  40% (10/25)        
remote: Counting objects:  44% (11/25)        
remote: Counting objects:  48% (12/25)        
remote: Counting objects:  52% (13/25)        
remote: Counting objects:  56% (14/25)        
remote: Counting objects:  60% (15/25)        
remote: Counting objects:  64% (16/25)        
remote: Counting objects:  68% (17/25)        
remote: Counting objects:  72% (18/25)        
remote: Counting objects:  76% (19/25)        
remote: Counting objects:  80% (20/25)        
remote: Counting objects:  84% (21/25)        
remote: Counting objects:  88% (22/25)        
remote: Counting objects:  92% (23/25)        
remote: Counting objects:  96% (24/25)        
remote: Counting objects: 100% (25/25)        
remote: Counting objects: 100% (25/25), done.        
remote: Compressing objects:  25% (1/4)        
remote: Compressing objects:  50% (2/4)        
remote: Compressing objects:  75% (3/4)        
remote: Compressing objects: 100% (4/4)        
remote: Compressing objects: 100% (4/4), done.        
remote: Total 29 (delta 21), reused 21 (delta 21), pack-reused 4        
From https://github.com/python/cpython
 * branch                  master     -> FETCH_HEAD
Reset branch 'master'

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1

make: *** No rule to make target `distclean'.  Stop.

[terryjreedy]

Error message for build 286 is

command timed out: 1200 seconds without output running ['./configure', '--prefix', '$(PWD)/target', '--with-pydebug'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=2048.933014
checking for linux/vm_sockets.h... 

Since patch did not add a C include, it is not obvious that this is really related. Since there have not been any subsequent merges and builds, I triggered a rebuild.
https://buildbot.python.org/all/#/builders/275/builds/287

[opavlyuk]

@terryjreedy thanks!

--- <exception caught here> ---
  File "/data/buildbot/venv/lib/python3.8/site-packages/buildbot/process/build.py", line 375, in startBuild
    ping_success_or_failure = yield workerforbuilder.ping()
twisted.spread.pb.PBConnectionLost: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionLost'>: Connection to the other side was lost in a non-clean fashion.
]

I have no idea how my patch could cause such things. It is likely an environment issue.

[terryjreedy]

The next build, 288, passed, so I think we are good for this issue.

[asvetlov]

Agree