Skip to content

Conversation

@Laerte
Copy link
Member

@Laerte Laerte commented Oct 3, 2021

@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
@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)

@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