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

Encode hostname directly instead of netloc #174

Merged
merged 10 commits into from
Jun 16, 2022
Merged

Encode hostname directly instead of netloc #174

merged 10 commits into from
Jun 16, 2022

Conversation

Laerte
Copy link
Member

@Laerte Laerte commented Oct 3, 2021

w3lib/url.py Outdated Show resolved Hide resolved
tests/test_url.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #174 (ff8338c) into master (d9763db) will decrease coverage by 0.06%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   95.68%   95.61%   -0.07%     
==========================================
  Files           7        7              
  Lines         463      479      +16     
  Branches       88       94       +6     
==========================================
+ Hits          443      458      +15     
  Misses         10       10              
- Partials       10       11       +1     
Impacted Files Coverage Δ
w3lib/url.py 97.71% <95.23%> (-0.32%) ⬇️

@Laerte Laerte requested a review from Gallaecio October 4, 2021 11:31
tests/test_url.py Outdated Show resolved Hide resolved
@Laerte
Copy link
Member Author

Laerte commented Oct 4, 2021

@Gallaecio The CI is failing on Build / build (3.9, pylint) (pull_request) but its not related to the code that i changed.

ERROR: InvocationError for command /home/runner/work/w3lib/w3lib/.tox/pylint/bin/pylint conftest.py docs setup.py tests w3lib (exited with code 16)

w3lib/url.py Outdated Show resolved Hide resolved
@Laerte Laerte requested a review from Gallaecio October 4, 2021 14:53
@Gallaecio
Copy link
Member

I need to delay my review until I have the time to carefully look into the potential issues with encoding and decoding each separate field.

@Gallaecio
Copy link
Member

@Laerte Sorry for the wait, I finally had a proper look.

I have created a pull request against your branch, https://github.com/Laerte/w3lib/pull/1, please have a look when you get a chance.

quote was indeed needed for username and password, but _safe_chars was not the right character allow-list for those fields.

safe_url_string: encode | and % in userinfo
@Laerte
Copy link
Member Author

Laerte commented Jan 3, 2022

@Gallaecio Thank you! Do you think we can merge this to master or need more changes?

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

It looks good to me. I’ll try and get someone else to have a look as well before we merge.

@Laerte
Copy link
Member Author

Laerte commented Jun 15, 2022

@wRAR Do you mind to take a look on this one? Thanks!

@wRAR wRAR merged commit 821dfe5 into scrapy:master Jun 16, 2022
@Laerte Laerte deleted the fix/encode-idna-domain-with-port branch June 16, 2022 16:54
@kmike kmike added this to the 2.0.0 milestone Aug 8, 2022
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