Demian Brecht added the comment:
> @demian: If you don't mind, could you please elaborate a bit more on
> `_resolve_path()` you mentioned in the review/comment?
Sure. In your patch, you have redirect_browser (or redirect if you
renamed it), which sounds like it's allowing for a very generic event to
happen: executing a redirect based on a passed in path. What I would
expect from calling into this is that the input path would be used as
the Location header in the resulting response. For example (using the
code as-is):
self.redirect_browser('http://example.com/foo/bar/')
The above would result in the response Location header being set to
input URL. What it's actually doing is a very specific thing:
Redirection if the final char in the input URL is '/', which is
something that I don't think needs to be exposed as part of the public API.
So, my recommendation is to do something like this:
def redirect(self, url, status=http.HTTPStatus.FOUND):
self.send_response(status)
self.send_header('Location', url)
self.end_headers()
Add a private helper method:
def _resolve_path(self, url):
parts = urllib.parse.urlsplit(url)
[...snip...]
return new_path_with_appended_slash
And in the body of send_head, do something like:
resolved_path = self._resolve_path(path)
if path != resolved_path:
self.redirect(resolved_path)
> As it stands, I have tried not to make any radical changes in existing
logic; This being my first patch and all.
The tough thing with adding public API in general is that you have to
consider both API readability as well as various use cases, not only the
one that you're specifically writing code for. To make your first patch
a little easier, you /could/ change the public API methods that you've
added to private helper methods and rename them as it makes sense. At
least in that way, you don't need to worry about making the methods
generic and writing tests for each where functionality is enhanced. In
reality, it seems that that's exactly what you're trying to achieve:
Helper methods where you're grouping logical bodies of code rather than
a full blown public API change (but I could be misunderstanding your
intentions there).
Hope that all makes sense.
----------
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue23255>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
