Issue 7059 - Modify fldiff so it can handle multiple files

abp/filters/diff_script.py

 # This file is part of Adblock Plus <https://adblockplus.org/>,
 # 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/>.
 
 """Command line script for rendering Adblock Plus filter list diffs."""
 
 from __future__ import print_function
 
 import argparse
 import io
 import sys
+import os.path

[Vasily Kuznetsov, 2018/10/24 11:30:09]

You can actually just `import os` here and `os.path.join` and other functions
will be available the same way.

[rhowell, 2018/10/26 21:42:21]

Done.

+import re
 
 from .renderer import render_diff
 
 __all__ = ['main']
 
 
-def parse_args():
+class MissingFilterListError(Exception):
+    """Error in finding a latest filter list, or base filter lists."""
+
+
+def parse_args(args_in=None):
     parser = argparse.ArgumentParser(description='Render a filter list diff.')
     parser.add_argument(

[Vasily Kuznetsov, 2018/10/24 11:30:09]

The formatting of this statement is different from two options recommended by
our style guide (see: https://adblockplus.org/coding-style#python). Somehow
slipped through in the previous review, but since it's changing now, there's a
good opportunity to bring it in line with the style guide.

The two recommended options (applied to this statement) would be like this:

    parser.add_argument('-o', '--output_dir',
                        help='The directory to write the diffs to',
                        default='-', nargs=1)

or this:

    parser.add_argument(
        '-o', '--output_dir', help='The directory to write the diffs to',
        default='-', nargs=1,
    )

In this case the first approach seems preferrable, and if we rearrange the
arguments a bit, we can get to an even more compact version like this:

    parser.add_argument('-o', '--output_dir', default='-', nargs=1,
                        help='The directory to write the diffs to')

[rhowell, 2018/10/26 21:42:19]

Done.

-        'base', help='the older filter list that needs to be updated',
-        nargs='?')
+        '-o', '--output_dir', help='The directory to write the diffs to',
+        default='-', nargs=1)

[Vasily Kuznetsov, 2018/10/24 11:30:09]

Since -o is now a directory, it can't default to `stdout`. Probably current
directory is a reasonable default.

[rhowell, 2018/10/26 21:42:18]

Is `os.getcwd()` the best way to do that? It makes running tox a little messy,
leaving the diff files in your current directory, but it's not hard to clean up.
I'll make sure to note that in the README.

     parser.add_argument(
-        'latest', help='the most recent version of the filter list',
-        nargs='?')
-    parser.add_argument(
-        'outfile', help='output file for filter list diff',
-        default='-', nargs='?')
-    return parser.parse_args()
+        '-l', '--latest', help='The most recent version of the filter list',
+        nargs=1)
+    return parser.parse_known_args(args_in)

[Vasily Kuznetsov, 2018/10/24 11:30:09]

Wouldn't it be better to add the positional arguments to the parser instead of
using parse_known_args? Kind of like in this example:
https://docs.python.org/3.4/library/argparse.html#example

[rhowell, 2018/10/26 21:42:19]

Done.

 
 
-def main():
+def main(args_in=None):
     """Entry point for the diff rendering script (fldiff)."""
-    args = parse_args()
+    arg = parse_args(args_in)
 
-    with io.open(args.base, 'r', encoding='utf-8') as base, \
-            io.open(args.latest, 'r', encoding='utf-8') as latest:
+    try:
+        if arg[0].latest is None:
+            raise MissingFilterListError('The --latest list is required')

[Vasily Kuznetsov, 2018/10/24 11:30:08]

This would be better handled by the arguments parser. We can either make this
argument required (`required=True`) or, even better, maybe it should become
positional, so the usage would be:

    fldiff [-o OUTPUT_DIR] latest base [base ...]

What do you think?

[rhowell, 2018/10/26 21:42:19]

Yeah, using a positional argument makes sense, and the usage looks nicer.

+        with io.open(arg[0].latest[0].strip(), 'r',
+                     encoding='utf8') as latest_list:
+            latest = latest_list.readlines()
+        if len(arg[1]) == 0:
+            raise MissingFilterListError('Archived lists are required')

[Vasily Kuznetsov, 2018/10/24 11:30:09]

This should also be handled by the arguments parser. You can use `nargs='+'` to
make a positional argument multivalued and requiring at least one.

[rhowell, 2018/10/26 21:42:18]

Done.

+        for base_file in arg[1]:
+            with io.open(base_file, 'r', encoding='utf8') as base:
+                out_dir = arg[0].output_dir[0].strip()

[Vasily Kuznetsov, 2018/10/24 11:30:08]

Do you think we need to strip the provided output dir?

For any spaces to be surrounding the argument value, the caller of the script
should really try hard to achieve it. Just having space in the command line will
not do this:

    $ fldiff -o foobar    -l 123.txt

They would need to quote the argument this way:

    $ fldiff -o 'foobar   ' -l 123.txt

And if they did it, perhaps there's a reason.

[rhowell, 2018/10/26 21:42:20]

Good point. For now, when testing with the dictionary, it does need to strip the
string, but once I remove that test, it's not necessary. I'll leave a note to
remove it later.

 
-        lines = render_diff(base, latest)
-        if args.outfile == '-':
-            outfile = io.open(sys.stdout.fileno(), 'w',
-                              closefd=False,
-                              encoding=sys.stdout.encoding or 'utf-8')
-        else:
-            outfile = io.open(args.outfile, 'w', encoding='utf-8')
-
-        with outfile:
-            for line in lines:
-                print(line, file=outfile)
+                lines = render_diff(base, latest)
+                if out_dir == '-':
+                    for line in lines:
+                        sys.stdout.write(line + '\n')
+                else:
+                    version = re.search(r'.*(\d+)\.txt', base_file).groups()[0]

[Vasily Kuznetsov, 2018/10/24 11:30:08]

I wonder if we can really depend on the version being contained in the filename.
Could it maybe be better to go through the first few lines using
`parse_filterlist()` (until we see the first line that is not a header and not
metadata) and get Version from metadata?

[rhowell, 2018/10/26 21:42:20]

Yeah, I wasn't sure how reliable this would be. Done.

+                    outfile = os.path.join(out_dir,
+                                           'diff{}.txt'.format(version))

[Vasily Kuznetsov, 2018/10/24 11:30:08]

The format of the output filename should probably be configurable. How about
adding a '-f FORMAT' argument with the default that you have here?

[rhowell, 2018/10/26 21:42:20]

Interesting idea. What did you have in mind? Like, a user can choose between
some predefined formats that we support? Or, they could define any format they
want?

+                    with io.open(outfile, 'w', encoding='utf-8') as out_fp:
+                        for line in lines:
+                            out_fp.write(line + '\n')
+    except MissingFilterListError as exc:
+        sys.exit(exc)