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 lookahead for ip check in AnyUrl #2512

Merged
merged 1 commit into from
May 9, 2021
Merged

Conversation

sbv-csis
Copy link
Contributor

@sbv-csis sbv-csis commented Mar 11, 2021

Change Summary

Added lookahead for the ip regex'es to allow for urls with 4 numbers preceeding the rest of the host

Related issue number

fix #1651

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #2512 (eb21f92) into master (619ff26) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #2512   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5109           
  Branches      1050      1050           
=========================================
  Hits          5109      5109           
Impacted Files Coverage Δ
pydantic/networks.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 619ff26...eb21f92. Read the comment docs.

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Thanks a lot for having a look at this! 🙏
Unfortunately there is a test case that used to pass and now fails. I suggest you add it

def test_ipv4_no_port():
    url = validate_url('ftp://123.45.67.8')
    assert url.scheme == 'ftp'
    assert url.host == '123.45.67.8'
    assert url.host_type == 'ipv4'
    assert url.port is None
    assert url.user is None
    assert url.password is None

changes/2512-sbv-csis.md Outdated Show resolved Hide resolved
@sbv-csis
Copy link
Contributor Author

Thanks a lot for having a look at this!
Unfortunately there is a test case that used to pass and now fails. I suggest you add it

def test_ipv4_no_port():
    url = validate_url('ftp://123.45.67.8')
    assert url.scheme == 'ftp'
    assert url.host == '123.45.67.8'
    assert url.host_type == 'ipv4'
    assert url.port is None
    assert url.user is None
    assert url.password is None

Thanks for the feedback and good catch!
I've added the test and updated the regex - I've made it to match the one used to end the domain group (minus the \s, I don't think that makes sense)

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

LGTM!

@sbv-csis
Copy link
Contributor Author

Should/could I do anything else here with this or are we on track for the next steps?

@GitHK
Copy link

GitHK commented Apr 27, 2021

Will this PR be merged, if yes is there a way to understand when it will be released?
Thanks.

@samuelcolvin
Copy link
Member

this is great, thank you so much.

@GitHK I'm aiming to make a new release over the coming weeks, but as you can see there are a lot of PRs to review. If you're hoping for a new release soon, perhaps you could speed things up by reviewing any on PRs with the "ready for review" badge?

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.

Validation Error on HttpUrl and AnyHttpUrl on parsing valid url starting with 4 numbers
4 participants