Demian Brecht added the comment:
Thanks for the extra effort on this to satisfy multiple people's opinions. It's
never an easy thing, especially when dealing with a decentralized group. My
following comments are largely based on the public API. I haven't done a full
review of the changes made to the tests.
All extraneous spaces should be removed. For example the first line here:
+
+ redirect_path = self._redirect_path(path)
+ if redirect_path:
> Lists the files we want the server to check and render to client. We can
> add/remove file names as required. Order of files denotes priority. Defaults
> to ['index.htm', 'index.html']
I think this might read a little better as:
"Overridable index files. If any of these files are present in a request
pointing to a directory on the server's file system, the contents of the file
will be returned. Otherwise, a directory listing will be returned. Defaults to
['index.htm', 'index.html']. Order of files denotes priority.
That said, I'm not a wordsmith so there may be a (much) better way of
structuring this.
> +class HTTPStatusType(Enum):
This doesn't sit quite well with me and I think it really should be removed.
There are very few cases (if any) that I can think of where this would be
useful. Cases where I've encountered the need for a range check generally is
around subsets (i.e. 200 and 201 mean entirely different things and /shouldn't/
be globbed together). Additionally (this is subjective), I find it a little
strange that the value of an enum is a range.
If for some reason it stays, I think the values should at least be sets
allowing for O(1) lookup.
> + def redirect(self, url, status=HTTPStatus.MOVED_PERMANENTLY):
There should be an assertion in this block that the status is one of the
redirect codes (301, 302, 308). Calling redirect('/foo', status=200) doesn't
make sense.
>+ if os.path.isfile(path):
+ return self.get_file(path)
+
+ # `path` has file with filename in self.index_files
+ for index_file in self.index_files:
+ index = os.path.join(path, index_file)
Before the directory-related logic executes, you could add a check in for
isdir() and short circuit with a 404 rather than relying on an index file not
being found and then running into list_directory(), which would result in a 404
with the message "No permission to list directory", which isn't actually
correct in the case where the directory doesn't exist.
> get_status_type, apply_success_headers, apply_headers
I think that these three methods should be removed as they're superfluous, make
the code less readable and is inconsistent with how responses and headers are
sent throughout the rest of server.py. The general practice is:
self.send_response()
self.send_header()
self.send_header()
self.end_headers()
The code in this patch should match that.
get_status_type is not needed as it /should/, if anything, be a single line
passthrough to a reverse lookup mapping. Iterating over each enum followed by
an iteration over each value is not optimal behaviour (O(N) rather than O(1)).
----------
_______________________________________
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
