GH-93880: Improved conciseness of ipaddress factory functions (GH-93917)#93985
GH-93880: Improved conciseness of ipaddress factory functions (GH-93917)#93985Nougat-Waffle wants to merge 3 commits intopython:mainfrom
Conversation
Nougat-Waffle
commented
Jun 18, 2022
Nougat-Waffle
commented
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
|
The issue number needs to be in the title of the PR, not the description of the PR |
Nougat-Waffle
changed the title
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
It doesn't seem to me that this PR fixes any bugs, and the extra import might make importing the module slower. Is there a strong rationale for this change?
You should also delete the news entry, since the changes this PR makes wouldn't be visible to end users in any way. I'll add the "skip news" label, which will mean the CI will still pass if you delete the news entry.
The rationale for this change is purely to increase the readability and elegancy of the code.
Done |
This is a worthy goal, but, in this case, I'm not sure it's worth the extra import. But the final decision needs to be made by the |
Sadly there are no active maintainers listed for |
ezio-melotti
left a comment
ezio-melotti
left a comment
There was a problem hiding this comment.
The changes seem reasonable, but I'm also not sure if it's worth merging it.
In theory this could be made even more concise with a function that does something like this:
def try_classes(address, classes, type, *args, **kwargs):
for cls in classes:
with contextlib.suppress(AddressValueError, NetmaskValueError):
return cls(address, *args, **kwargs)
raise ValueError(f'{address!r} does not appear to be an IPv4 or IPv6 {type}')or like this (if we want to get rid of the extra import):
def try_classes(address, classes, type, *args, **kwargs):
for cls in classes:
try:
return cls(address, *args, **kwargs)
except (AddressValueError, NetmaskValueError):
pass
raise ValueError(f'{address!r} does not appear to be an IPv4 or IPv6 {type}')but again I'm not sure if it's worth the extra complexity just to save a few lines
|
Closing on the basis that:
Thanks for the PR, but we'll decline this one :) |
4 participants
