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

Add IpAddr <-> ipaddress.IPv(4/6)Address conversion #3197

Merged
merged 1 commit into from
Jun 12, 2023

Conversation

mhils
Copy link
Contributor

@mhils mhils commented Jun 1, 2023

This PR adds support for conversion between Python's ipaddress.IPv4Address/ipaddress.IPv6Address and std::net::IpAddr. 😃

src/conversions/std/ipaddr.rs Outdated Show resolved Hide resolved
@mhils
Copy link
Contributor Author

mhils commented Jun 1, 2023

(Please note that you'll either need to squash or rebase into a coherent history at the end before meging.)

To clarify, does this mean that your automation does not squash automatically an I am expected squash myself manually? Happy to do that, I didn't find anything specific in Contributing.md and expected you'd prefer to just press GitHub's squash merge button in the end. :)

@DataTriny
Copy link
Contributor

DataTriny commented Jun 1, 2023

First time contributor has agreed to the new licensing scheme.

@adamreichold
Copy link
Member

(Please note that you'll either need to squash or rebase into a coherent history at the end before meging.)

To clarify, does this mean that your automation does not squash automatically an I am expected squash myself manually? Happy to do that, I didn't find anything specific in Contributing.md and expected you'd prefer to just press GitHub's squash merge button in the end. :)

Our automation is currently bors and it can only always merge or always squash. Since we do like coherent multi-commit histories, we do not squash and hence you need to either squash manually or provide a coherent multi-commit history.

(This is not currently documented, but it is probably not useful ATM as bors-ng will not available for long and we will need to switch to GitHub's merge queues (which seem to have the same issue) soon.)

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

The extract implementation is somewhat broad, but I think this is alright since the conversion are guided by the given types on the Rust side of things.

src/sync.rs Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member

CI fails due to any unused import in the sync module.

@mhils
Copy link
Contributor Author

mhils commented Jun 8, 2023

CI fails due to any unused import in the sync module.

Fixed, thanks! 🙇

@adamreichold
Copy link
Member

@davidhewitt I would want to merge this but would be glad for a second opinion on the broadness of the extract implementation.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Yep, this looks great to me - thanks both and sorry for the delay on my part!

@davidhewitt
Copy link
Member

I think we're happy to see this move forward, so I'll click merge now. Thanks again!

@davidhewitt davidhewitt added this pull request to the merge queue Jun 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 11, 2023
@mhils
Copy link
Contributor Author

mhils commented Jun 11, 2023

Thank you two again! 🍰

It seems like the merge queue has hit an unrelated race condition, happy to fix things here if I'm misreading it. :)

@davidhewitt
Copy link
Member

Yep no worries I will get #3225 merged and then retry.

@davidhewitt davidhewitt added this pull request to the merge queue Jun 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 11, 2023
@adamreichold adamreichold added this pull request to the merge queue Jun 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 12, 2023
@davidhewitt davidhewitt added this pull request to the merge queue Jun 12, 2023
Merged via the queue into PyO3:main with commit 199261f Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants