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

feat: add ipnet support #3710

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

BeauGieskens
Copy link

Fixes #2150.

I chose to favour ipnet over ipnetwork if both features are used, because it seems more widely used and actually has a stable release.

@BeauGieskens
Copy link
Author

Forgive me, it's my first time making a serious pull request to this project. I'll find some time later to recreate the failing tests locally and fix what I can.

@abonander
Copy link
Collaborator

I chose to favour ipnet over ipnetwork if both features are used, because it seems more widely used and actually has a stable release.

That actually makes it a breaking change, which would mean waiting until 0.9.0.

@BeauGieskens
Copy link
Author

That actually makes it a breaking change, which would mean waiting until 0.9.0.

I see that it's breaking, but I figure it's only if someone is already trying to use the currently non-existent ipnet feature alongside ipnetwork. Unless I'm missing something.

Either way, it's fine with me if this waits until 0.9.0 :)

@abonander
Copy link
Collaborator

Because of Cargo's feature unification, if any crate that depends on SQLx in the dependency tree turns on a feature, it is forced on for all crates that depend on it.

This has caused issues before: #3412 (comment)

Thus it's probably best that it doesn't take precedent unless it needs to. I'm working on a way around this in #3383 but it's better if the user doesn't need to do this in the first place. If it's the other way around and they want to use ipnet over ipnetwork then it's likely something they're specifically opting into, not a breakage they have to fix.

@BeauGieskens
Copy link
Author

Thanks for the helpful explanation, I learned something new today and I agree with your preference.

I've swapped it around so ipnetwork is preferred. It's just as well because I noticed a spot where I previously had the precedence the wrong way around.

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.

Support ipnet along with ipnetwork
2 participants