Issue 7019 - [CMS] Refactor `test_server.py`

cms/bin/test_server.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-present eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 from __future__ import print_function
 
 import os
 import mimetypes
-from argparse import ArgumentParser

[Vasily Kuznetsov, 2018/10/22 14:36:47]

In some cases we get easier to follow code by importing modules and then using
the dotted notation (e.g. import os --> foo = os.path.join(bar, baz)) as opposed
to importing the functions and classes (e.g. from os.path import join --> foo =
join(bar, baz)). Google style guide [1] specifically recommends the former
approach and I'm starting to think that they are right. We don't have anything
about this in our style guide though.

I see you replaced the module import with a class import here. My first impulse
is to say that this is in contradiction to the thinking above and is not
desirable. However, I can also see that it's pretty clear where ArgumentParser
is coming from, so any confusion is unlikely. Still, there's an argument for a
consistent style. So at the end of the day... it seems complicated :/.

What was your thinking when you changed this?

[1]: https://github.com/google/styleguide/blob/gh-pages/pyguide.md#22-imports

[Tudor Avram, 2018/10/22 17:29:38]

Well, basically the idea was to make the entire thing less verbose. As you said,
it seemed to me that `ArgumentParser` was obvious enough and there was need to
write `argparse.ArgumentParser`. 

But yeah, I guess I can just change that to comply with the standards.

[Vasily Kuznetsov, 2018/10/23 10:16:14]

I see. Well, at the moment there's no standard about this and perhaps we should
think if such standard would make sense (let me know what you think) and, if it
does, introduce it. As for now both styles are ok, so you can leave it as is.

[Tudor Avram, 2018/10/23 16:44:33]

I guess it's more important to make it obvious where classes/ methods comes
from. In consequence, I changed this to `import argparse` and then using
`argparse.ArgumentParser(...)`

+import argparse
 
 import jinja2
 
 from cms.converters import converters
 from cms.utils import process_page
 from cms.sources import create_source
 
 UNICODE_ENCODING = 'utf-8'
 
 ERROR_TEMPLATE = '''
@@ skipping 41 matching lines @@
     def _get_data(self, path):
         """Read the data corresponding to a website path.
 
         Parameters
         ----------
         path: str
             The path to the page to get the data for.
 
         Returns
         -------
-        str/ bytes

[Vasily Kuznetsov, 2018/10/22 14:36:46]

Numpy docstring standard [1], that we use here seems to suggest using literal
"or" in such cases, so it would be "str or bytes".

[1]: https://numpydoc.readthedocs.io/en/latest/format.html#sections

[Tudor Avram, 2018/10/23 16:44:32]

Done.

+        str or bytes
             The data corresponding to the path we're trying to access.
 
         """
         if self.source.has_static(path):
             return self.source.read_static(path)
 
         page, data = self._get_page(path)
 
         if page and self._has_conflicts(page):
             raise Exception('The requested page conflicts with another page.')
 
         return data
 
     def _get_page(self, path):
         """Construct a page and return its contents.
 
         Parameters
         ----------
         path: str
             The path of the page we want to construct.
 
         Returns
         -------
-        (str, str)

[Vasily Kuznetsov, 2018/10/22 14:36:46]

Numpy docstring standard (link in the previous comment) has two possible formats
for the return. I think here we could use the second one nicely:

    Returns
    --------
    (page_name, page_contents) : (str, str)
        The name of the page and its rendered contents.

[Tudor Avram, 2018/10/23 16:44:32]

Done.

-            With the following format:
-                <page_name, page_contents>
+        (page_name, page_contents): (str, str)
 
         """
         path = path.strip('/')
         if path == '':
             locale, page = self.source.read_config().get('general',
                                                          'defaultlocale'), ''
         elif '/' in path:
             locale, page = path.split('/', 1)
         else:
             locale, page = path, ''
 
         default_page = self.source.read_config().get('general', 'defaultpage')
         possible_pages = [page, '/'.join([page, default_page]).lstrip('/')]
 
         for page_format in converters.iterkeys():
             for p in possible_pages:
                 if self.source.has_page(p, page_format):
                     return p, process_page(self.source, locale, p, page_format,
                                            self.full_url)
 
         if self.source.has_localizable_file(locale, page):
             return page, self.source.read_localizable_file(locale, page)
 
         return None, None
 
     def _has_conflicts(self, page):
         """Check if a page has conflicts.
 
         A page has conflicts if there are other pages with the same name.
+
         Parameters

[Vasily Kuznetsov, 2018/10/22 14:36:46]

Nit: there should be an empty line before the header

[Tudor Avram, 2018/10/23 16:44:32]

Done.

         ----------
         page: str
             The path of the page we're checking for conflicts.
 
         Returns
         -------
         bool
             True - if the page has conflicts
             False - otherwise
 
@@ skipping 11 matching lines @@
     def get_error_page(self, start_response, status, **kw):
         """Create and display an error page.
 
         Parameters
         ----------
         start_response: function
             It will be called before constructing the error page, to setup
             things like the status of the response and the headers.
         status: str
             The status of the response we're sending the error page with.
-            Needs to have the following format: "<status_code>

[Vasily Kuznetsov, 2018/10/22 14:36:47]

It would be better to not break the line the middle of this quoted string,
otherwise it's not very clear what exactly the format is. Maybe we just move the
whole thing to the next line?

[Tudor Avram, 2018/10/23 16:44:30]

Done.

-            <status_message>"
+            Needs to have the following format:
+                "<status_code> <status_message>"
         kw: dict
             Any additional arguments that will be passed onto the `stream`
-            method
-            of a `jinja2 Template`.

[Vasily Kuznetsov, 2018/10/22 14:36:47]

Nit: redundant line break above?

[Tudor Avram, 2018/10/23 16:44:31]

Done.

-
-        Returns
-        -------
-        generator
-            of utf8 strings - fragments of the corresponding error HTML

[Vasily Kuznetsov, 2018/10/22 14:36:47]

I think "of utf8 strings" should be part of the "type" here. I've seen such
"human-readable" types used a lot in NumPy style docstrings and I think they
work pretty well.

[Tudor Avram, 2018/10/23 16:44:33]

Done.

-            template.
+            method of a `jinja2 Template`.
+
+        Returns
+        -------
+        generator of utf8 strings
+            Fragments of the corresponding error HTML template.
 
         """
         env = jinja2.Environment(autoescape=True)
         page_template = env.from_string(ERROR_TEMPLATE)
         mime = 'text/html; encoding={}'.format(UNICODE_ENCODING)
 
         start_response(status, [('Content-Type', mime)])
 
         for fragment in page_template.stream(status=status, **kw):
             yield fragment.encode(UNICODE_ENCODING)
 
     def __call__(self, environ, start_response):
-        """Execute the handler, according with the WSGI standards.

[Vasily Kuznetsov, 2018/10/22 14:36:47]

Nit: according to the internet the normal form is "according to" or "in
accordance with" but not "according with".

[Tudor Avram, 2018/10/23 16:44:33]

Done.

+        """Execute the handler, according to the WSGI standards.
 
         Parameters
         ---------
         environ: dict
             The environment under which the page is requested.
             The requested page must be under the `PATH_INFO` key.
         start_response: function
             Used to initiate a response. Must take two arguments, in this
             order:
                 - Response status, in the format "<code> <message>".
                 - Response headers, as a list of tuples.
 
         Returns
         -------
-        list

[Vasily Kuznetsov, 2018/10/22 14:36:46]

It's "list of str", right? Perhaps we can add that to the type.

[Tudor Avram, 2018/10/23 16:44:31]

Done.

+        list of str
             With the data for a specific page.
 
         """
         path = environ.get('PATH_INFO')
 
         data = self._get_data(path)
 
         if data is None:
             return self.get_error_page(start_response, '404 Not Found',
                                        uri=path)
 
         mime = mimetypes.guess_type(path)[0] or 'text/html'
 
         if isinstance(data, unicode):
             data = data.encode(UNICODE_ENCODING)
             mime = '{0}; charset={1}'.format(mime, UNICODE_ENCODING)
 
         start_response('200 OK', [('Content-Type', mime)])
         return [data]
 
 
-def get_handler():

[Vasily Kuznetsov, 2018/10/22 14:36:47]

I was imagining a structure where we parse the arguments in this function and
return `args` that are be used in `main()` to instantiate the server. Bundling
the two together feels a bit strange. What do you think about this approach?

[Tudor Avram, 2018/10/23 16:44:30]

Done.

-    """Set the arguments required to run the script.
+def parse_arguments():
+    """Set up and parse the arguments required by the script.
 
     Returns
     -------
-    TestServerHandler
-        Initialised with the script parameters.
+    argparse.Namespace
+        With the script arguments, as parsed.
 
     """
-    parser = ArgumentParser(description='CMS development server created to '
-                                        'test pages locally and on-the-fly')
+    parser = argparse.ArgumentParser(
+        description='CMS development server created to test pages locally and '
+                    'on-the-fly.',
+    )
 
     parser.add_argument('path', default=os.curdir, nargs='?',
                         help='Path to the website we intend to run. If not '
                              'provided, defaults, to the current directory.')
     parser.add_argument('--host', default='localhost',
                         help='Address of the host the server will listen on. '
                              'Defaults to "localhost".')
     parser.add_argument('--port', default=5000, type=int,
                         help='TCP port the server will listen on. Default '
                              '5000.')
 
-    args = parser.parse_args()
-
-    return DynamicServerHandler(args.host, args.port, args.path)
+    return parser.parse_args()
 
 
 def run_werkzeug_server(handler, **kw):
     """Set up a server that uses `werkzeug`.
 
     Parameters
     ----------
     handler: DynamicServerHandler
         Defines the parameters and methods required to handle requests.
 
     Raises
     ------
     ImportError
         If the package `werkzeug` is not installed
 
     """
     from werkzeug.serving import run_simple
+    import logging
 
     def run(*args, **kwargs):
         # The werkzeug logger must be configured before the
         # root logger. Also we must prevent it from propagating
         # messages, otherwise messages are logged twice.
-        import logging

[Vasily Kuznetsov, 2018/10/22 14:36:46]

Is it necessary to import logging here and not at the top? Seems a bit weird,
doesn't it?

[Tudor Avram, 2018/10/22 17:29:38]

It's not used anywhere else in the script, that's why I put it here (it would
only be imported if `werkzeug` is installed when running the script. Thus, it
wouldn't be unnecessarily imported.

[Vasily Kuznetsov, 2018/10/23 10:16:14]

I see. Importing `logging` is probably not a big deal performance-wise, but
yeah, it kind of makes sense. What if we move the import to line 273 instead,
just after the werkzeug import?

[Tudor Avram, 2018/10/23 16:44:31]

Done.

         logger = logging.getLogger('werkzeug')
         logger.propagate = False
         logger.setLevel(logging.INFO)
         logger.addHandler(logging.StreamHandler())
 
         run_simple(threaded=True, *args, **kwargs)
 
     run(handler.host, handler.port, handler, **kw)
 
 
@@ skipping 23 matching lines @@
                 )
 
         server = make_server(host, port, wrapper, ThreadedWSGIServer)
         print(' * Running on {0}:{1}'.format(*server.server_address))
         server.serve_forever()
 
     run(handler.host, handler.port, handler, **kw)
 
 
 def main():
-    handler = get_handler()
+    args = parse_arguments()
+    handler = DynamicServerHandler(args.host, args.port, args.path)
 
     try:
         run_werkzeug_server(handler, use_reloader=True, use_debugger=True)
     except ImportError:
         run_builtins_server(handler)
 
 
 if __name__ == '__main__':
     main()