[issue23255] SimpleHTTPRequestHandler refactor for more extensible usage.

[Demian Brecht]

Demian Brecht added the comment:

Thanks for the work! I'm not sure why the last patch doesn't appear on Rietveld, so (unfortunately) here's the result of my review. I've only covered functional aspects in this run at it:


+    base_files = ['index.html', 'index.htm']

Can you use "index_files"? That's the commonly used term to refer to these files.

-    def copyfile(self, source, outputfile):
+    def copy_file(self, source, outputfile):

As Berker mentioned, we can't just rename this as it's not backwards compatible. Standard library modules aren't the only dependency. Such a change would cause breakage in, say, any 3rd party code that subclasses SimpleHTTPRequestHandler. This should remain as copyfile.

+    def redirect_browser(self, path, parts):

Can "_browser" be dropped here? This applies to all clients, not only browsers. Additionally, I think that the name is a little misleading. I think it would be better to have a generic public redirect(<url>, status=http.FOUND) method and then an internal _resolve_path() that calls into the redirect method using the Apache-like logic. It also seems like the path parameter is unused so should be dropped.

+    def get_path_or_dir(self, path):

I think the content of this method should be changed to result in consistent output. Right now, you're either returning a file path /or/ a BytesIO object containing the full output of the directory listing. It might make sense to have a single method that takes the path and produces consistent BytesIO object, regardless of whether it's a directory or a file path.

+            self.send_response(301)

Please use the http.HTTPStatus enum for status codes (i.e. http.HTTPStatus.MOVED_PERMANENTLY)

+    def apply_headers(self, f, path)

I don't think that this makes sense as a public API as it is as it only accounts for a 200 response. What if any error conditions are raised with the given file? As this is functionality specific to the single case in which it's used, I think this should either be left as-is, made more generic to handle header values based on any potential state of the file object, or made into a private helper method (indicated by a single underscore prefix to the method name).

Also, you should be able to derive the path from the file parameter (f.name), so I'm not sure that the extra path parameter is necessary.

+    def get_response(self, tail=False)

tail should default to None here, otherwise it's a little confusing as to why a parameter that /looks/ like it should be a bool actually expects a string value.

----------

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue23255>
_______________________________________