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

[test_url_query] Add failing test for str() #233

Closed
wants to merge 1 commit into from

Conversation

behnam
Copy link
Contributor

@behnam behnam commented Aug 21, 2018

Add failing test for URL.__str__() on an instance with embedded URL
as query value.

Add failing test for `URL.__str__()` on an instance with embedded URL
as query value.
@behnam
Copy link
Contributor Author

behnam commented Aug 21, 2018

This test demonstrates the main blocker we have on yarl. I was trying to get this out as port of #220, but since it's taking longer to land that, adding it here in a minimal diff.

This asymmetric behavior is different from the issue with the current API design for query value getter/setter, which returns/expects encoded values.

What do you think?

@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #233 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #233   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files           2        2           
  Lines         584      584           
  Branches      140      140           
=======================================
  Hits          582      582           
  Partials        2        2

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 84a9a2b...f646565. Read the comment docs.

@serhiy-storchaka
Copy link
Contributor

It is easy to make this test passing. But is not it too strict? What is wrong with the current behavior? If preserve encoded ? in raw_query_string, which other encoded characters should be preserved? And in which other parts of URL? If preserve %3F, should %3f also be preserved?

The following test is passed.

assert URL(str(u)) == u

@mjpieters
Copy link
Contributor

I'm not sure there is real value in this PR, and nothing has happened with it in nearly 2 years so I'm closing this. Feel free to recreate or request reopening if this is still something to pursue.

@mjpieters mjpieters closed this Jun 14, 2022
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.

5 participants