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

Remove octal/hexadecimal parsing from IPv4. Fixes #9187 #19811

Merged
merged 2 commits into from
Jan 25, 2017

Conversation

Keno
Copy link
Member

@Keno Keno commented Jan 2, 2017

No description provided.

@tkelman tkelman added the needs news A NEWS entry is required for this change label Jan 2, 2017
@@ -148,6 +148,11 @@ end

# Parsing

const ipv4_leading_zero_error = """
Leading zeros in IPv4 addresses are disallowed due to ambiguity.
If octal or hexadecimal, convert to decimal, otherwise remove leading zero.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this reads kind of weird, since the first sentence is a complete sentence and this one isn't. Perhaps the second could be rephrased as

If the address is in octal or hexadecimal, convert it to decimal, otherwise remove the leading zero.

?

@ararslan
Copy link
Member

ararslan commented Jan 3, 2017

This is technically a breaking change, right? So I guess it would have to be noted as such in NEWS?

@JeffBezanson JeffBezanson added the breaking This change will break code label Jan 6, 2017
@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Jan 24, 2017
@StefanKarpinski
Copy link
Member

Marking as 0.6.0 since this seems mostly ready to go.

@tkelman
Copy link
Contributor

tkelman commented Jan 24, 2017

Would be gentler to have this go through a deprecation cycle, if possible.

@StefanKarpinski
Copy link
Member

I think uses of this syntax are so incredibly rare and marginal that we can just remove support.

@tkelman
Copy link
Contributor

tkelman commented Jan 25, 2017

@ararslan do you want to push a rewording here then?

@ararslan
Copy link
Member

@tkelman The deed is done.

@StefanKarpinski StefanKarpinski merged commit 5087222 into master Jan 25, 2017
@StefanKarpinski StefanKarpinski deleted the kf/removeipoh branch January 25, 2017 22:00
@StefanKarpinski
Copy link
Member

Needs a NEWS entry in the breaking changes section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants