Skip to content

gh-101531: Handle omitted leading zeroes in mac address on Darwin (#1… #105004

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evgeniiz321
Copy link

@evgeniiz321 evgeniiz321 commented May 26, 2023

@ghost
Copy link

ghost commented May 26, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@evgeniiz321 evgeniiz321 force-pushed the gh-101531 branch 4 times, most recently from d7ad703 to edbb512 Compare May 27, 2023 16:27
Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some additional tests as well,

def check_parse_mac(self, aix):

This needs to be changed to handle more than just 'aix' and 'not aix', as well as adding a new test_parse_mac_macos method.

https://github.com/python/cpython/blob/fb02b6696002fc3dedbcf7c58aa40301650936ba/Lib/test/test_uuid.py#L894C9-L894C35

Likewise an addition to also test update to deal with the new behaviour (even TBH this should have been done while adding AIX support to the function), just add the a variant that tests with macOS values and settings.

Lib/uuid.py Outdated
@@ -427,7 +429,7 @@ def _find_mac_near_keyword(command, args, keywords, get_word_index):
if words[i] in keywords:
try:
word = words[get_word_index(i)]
mac = int(word.replace(_MAC_DELIM, b''), 16)
mac = _parse_mac(word)
except (ValueError, IndexError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
except (ValueError, IndexError):
except IndexError:

_parse_mac should not raise ValueError, but returns None on invalid values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Lib/uuid.py Outdated
@@ -66,6 +66,8 @@
if _AIX:
_MAC_DELIM = b'.'
_MAC_OMITS_LEADING_ZEROES = True
if sys.platform in ('darwin', ):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if sys.platform in ('darwin', ):
if sys.platform == 'darwin':

Using in to test the value is not necessary at this moment, can be changed when there is another platform that needs this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@bedevere-app
Copy link

bedevere-app bot commented Dec 27, 2023

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ronaldoussoren ronaldoussoren self-assigned this Dec 27, 2023
@evgeniiz321 evgeniiz321 force-pushed the gh-101531 branch 3 times, most recently from 3e4b47d to de560d0 Compare March 31, 2024 18:02
@evgeniiz321
Copy link
Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Mar 31, 2024

Thanks for making the requested changes!

@ronaldoussoren: please review the changes made to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants