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

Inconsistent behaviour between Cython and CPython (or lack of docs, or both) #991

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pajod
Copy link

@pajod pajod commented Feb 7, 2024

What do these changes do?

Adds a test for the quoting inconsistency that I believe needs change. It is written such that the compiled module passes but setting YARL_NO_EXTENSIONS=1 fails.

Submitting without the desired change to verify CI results. May or may not add actual content eventually.

Are there changes in behavior for the user?

No, but maybe there should be. Telling the user they are responsible about URL correctness is not particularly useful if the specific correctness properties are not enumerated.

Related issue number

Still roughly the same scenario, just moved from aio-libs/aiohttp#8088 where it was unnecessarily tested for, and thus failed on PyPy

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@webknjaz
Copy link
Member

We should really finish up #860 to catch the differences better...

assert u.query_string == ""
assert u.fragment == "Pünktelchen\udca0\udcef\udcb6"
assert u.path == u2.path
# assert u.fragment == u2.fragment
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# assert u.fragment == u2.fragment

@@ -584,3 +584,34 @@ def test_empty_path(self):
assert u.path == ""
assert u.query_string == ""
assert u.fragment == ""

def test_truncated_utf_sequence(self):
Copy link
Member

Choose a reason for hiding this comment

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

If path is the only difference, could you parametrize the test?

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.

None yet

2 participants