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 InetAddr constructor from string #31459

Closed
wants to merge 4 commits into from
Closed

add InetAddr constructor from string #31459

wants to merge 4 commits into from

Conversation

piever
Copy link
Contributor

@piever piever commented Mar 23, 2019

This makes it easier to write code that would work with either a String or a IPAddr. Some packages like HTTP write their own internal method for this (https://github.com/JuliaWeb/HTTP.jl/blob/master/src/Servers.jl#L184) but I think something like InetAddr("127.0.0.1", 8000) is sufficiently unambiguous that it should have its own official constructor.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Seems good to me.

@ararslan ararslan added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Mar 26, 2019
@piever
Copy link
Contributor Author

piever commented Apr 3, 2019

I've added a news entry. I think documentation should be added in a separate PR as here I'm just adding a constructor, the type existed already (unexported and undocumented).

EDIT: nevermind, now I see the concern. If I don't add docs here it with compat annotation it'll be hard to tell in the future that one of the constructors was added afterwards. I've just added docs and compat annotation.

stdlib/Sockets/src/IPAddr.jl Outdated Show resolved Hide resolved
stdlib/Sockets/src/IPAddr.jl Outdated Show resolved Hide resolved
stdlib/Sockets/src/IPAddr.jl Outdated Show resolved Hide resolved
@piever
Copy link
Contributor Author

piever commented Apr 3, 2019

Thanks for the feedback, I've included your suggestions!

@JeffBezanson
Copy link
Member

Changed version compat to 1.3, squashed and merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs compat annotation Add !!! compat "Julia x.y" to the docstring needs docs Documentation for this change is required needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants