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

Set the default path_info to the root '/' #768

Closed
wants to merge 8 commits into from

Conversation

@zhaoguixu
Copy link
Contributor

@zhaoguixu zhaoguixu commented Sep 9, 2015

Set the default path_info to the root '/' when both the path_info passed to 'bind' and 'match' are None.
#740

@untitaker
Copy link
Member

@untitaker untitaker commented Apr 10, 2016

@zhaoguixu Those tests fail for me. Could you take a look? Thanks!

@untitaker untitaker self-assigned this Apr 10, 2016
Fixed the new added test ``` test_both_bind_and_match_path_info_are_none``` to avoid failures
Update test_routing.py by fixing new added test  ```test_both_bind_and_match_path_info_are_none```
@zhaoguixu
Copy link
Contributor Author

@zhaoguixu zhaoguixu commented Apr 11, 2016

This should be good. @untitaker

@untitaker
Copy link
Member

@untitaker untitaker commented Apr 11, 2016

Please run make stylecheck on your code, some things are not conforming to the styleguide.

Will review content later.

@@ -1498,7 +1498,7 @@ def match(self, path_info=None, method=None, return_rule=False,
"""
self.map.update()
if path_info is None:
path_info = self.path_info
path_info = self.path_info or u'/'

This comment has been minimized.

@untitaker

untitaker Apr 11, 2016
Member

Perhaps this logic would be better suited for bind, such that self.path_info is not None?

Something like:

diff --git a/werkzeug/routing.py b/werkzeug/routing.py
index 82cf793..888ac9b 100644
--- a/werkzeug/routing.py
+++ b/werkzeug/routing.py
@@ -1232,6 +1232,8 @@ class Map(object):
             subdomain = self.default_subdomain
         if script_name is None:
             script_name = '/'
+        if path_info is None:
+            path_info = '/'
         server_name = _encode_idna(server_name)
         return MapAdapter(self, server_name, script_name, subdomain,
                           url_scheme, path_info, default_method, query_args)
@@ -1498,7 +1500,7 @@ class MapAdapter(object):
         """
         self.map.update()
         if path_info is None:
-            path_info = self.path_info or u'/'
+            path_info = self.path_info
         else:
             path_info = to_unicode(path_info, self.map.charset)
         if query_args is None:
@untitaker
Copy link
Member

@untitaker untitaker commented Apr 11, 2016

I don't really understand why you need this behavior in the first place. In which situation would you call match without arguments?

@zhaoguixu
Copy link
Contributor Author

@zhaoguixu zhaoguixu commented Apr 11, 2016

Both the default value of path_info of method bind and match is None. This give us a sense None is acceptable. However, at most one of the two can accept None.

@untitaker
Copy link
Member

@untitaker untitaker commented Apr 11, 2016

I think it was intended to just have no implicit default at all. That this is expressed with a 404 is unfortunate.

@zhaoguixu
Copy link
Contributor Author

@zhaoguixu zhaoguixu commented Apr 11, 2016

I am not very familiar with the whole logic. I just test the two combined in one situation and it gives me a trackback. Feel free to decide whether this fix is necessary.

Adjust styles.
@untitaker untitaker assigned mitsuhiko and unassigned untitaker Apr 13, 2016
@untitaker
Copy link
Member

@untitaker untitaker commented Apr 13, 2016

Delegating to @mitsuhiko, because I don't really know.

@davidism
Copy link
Member

@davidism davidism commented May 28, 2018

continued in #1316

@davidism davidism closed this May 28, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants