Skip to content

Conversation

@cansarigol
Copy link
Contributor

No description provided.

httpx/models.py Outdated
self._uri_reference = self._uri_reference.copy_with(
authority=idna_authority
)
) # pragma: nocover
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of disabling coverage here let's create a simple unit test for the branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a test case because if I try to Unicode domain, I get this error: rfc3986.exceptions.InvalidAuthority: The authority (😁.de) is not valid.

Should we change this part if there is no case or could you help me with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about ουτοπία.δπθ.gr? Emojis aren't valid for IDNs for IDNA 2008.

Copy link
Contributor Author

@cansarigol cansarigol Aug 14, 2019

Choose a reason for hiding this comment

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

idna encode raises an error if there is any char that greater then 128. (it's not just about emoji link) so this if clause is never used. If you want we can delete this part with another PR and check InvalidAuthority error?

Copy link
Contributor

@sethmlarson sethmlarson Aug 14, 2019

Choose a reason for hiding this comment

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

I think we can literally just get rid of that whole if statement branch. IRIReference.encode() does everything for us. Tested it locally and it seems to work for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a pr #210

httpx/utils.py Outdated

if netrc_path is None:
return None
return None # pragma: nocover
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also create a unit test for here?

@sethmlarson sethmlarson merged commit 9536b76 into encode:master Aug 15, 2019
@sethmlarson
Copy link
Contributor

Thanks @cansarigol!

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.

2 participants