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

Yarl in reverse urls #1288

Merged
merged 12 commits into from
Oct 6, 2016
Merged

Yarl in reverse urls #1288

merged 12 commits into from
Oct 6, 2016

Conversation

asvetlov
Copy link
Member

@asvetlov asvetlov commented Oct 5, 2016

No description provided.

@@ -296,7 +264,11 @@ def get_info(self):
return {'path': self._path}

def url(self, *, query=None):
return self._append_query(self._path, query)
super().url()
Copy link
Member

Choose a reason for hiding this comment

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

Warning here will suggest to use url_for, but url_for doesn't accepts query parameter. So deprecation message should be a bit different or it will cause confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

url_for returns URL, not str.
But functionally it's a replacement for deprecated API.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I see that. I mean that simple replace url(...) with url_for as message says wouldn't be enough for upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe a user will go to documentation chapter about url_for and will figure out how to use new API properly.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's hope for that (:

Copy link
Contributor

@alexhayes alexhayes Oct 20, 2016

Choose a reason for hiding this comment

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

Where is the documentation for url_for? Nevermind, found it

@codecov-io
Copy link

codecov-io commented Oct 6, 2016

Current coverage is 98.41% (diff: 100%)

Merging #1288 into master will decrease coverage by 0.08%

@@             master      #1288   diff @@
==========================================
  Files            29         29          
  Lines          6520       6503    -17   
  Methods           0          0          
  Messages          0          0          
  Branches       1089       1094     +5   
==========================================
- Hits           6422       6400    -22   
- Misses           47         52     +5   
  Partials         51         51          

Powered by Codecov. Last update a411219...417e491

@asvetlov asvetlov merged commit 53c3687 into master Oct 6, 2016
@asvetlov asvetlov deleted the yarl_in_reverse_urls branch October 6, 2016 16:00
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants