Issue 29922555: Issue 7059 - Modify fldiff so it can handle multiple files (Closed)
|
Created:
Oct. 24, 2018, 2:27 a.m. by rhowell Modified:
Oct. 31, 2018, 9:14 p.m. Reviewers:
Vasily Kuznetsov Base URL:
https://hg.adblockplus.org/python-abp Visibility:
Public. |
DescriptionIssue 7059 - Modify fldiff so it can handle multiple files
Patch Set 1 #
Total comments: 31
Patch Set 2 : Address comments on PS1 #
Total comments: 9
Patch Set 3 : Address comments on PS2 #
Total comments: 6
Patch Set 4 : Address comments on PS3 #
Total comments: 4
Patch Set 5 : Address comments on PS4 #
MessagesTotal messages: 13
I left the test with the dictionary in there for now. I'll remove it before pushing any code, it just makes testing a bit easier for me.
Hi Rosie, The overall direction looks good but I have quite a few comments and proposals on the details, as you will see ;) One general remark: I proposed in the comments on the code to remove the mode with output to stdout. It doesn't really work with multiple base files and supporting it for the case where we have only one base file makes the command line interface less consistent and more complicated. Could you please check with Paco that the Ops don't need the stdout mode before we remove it -- if they do see a use for it already, perhaps we should reverse this decision and support it after all. Cheers, Vasily https://codereview.adblockplus.org/29922555/diff/29922556/README.md File README.md (right): https://codereview.adblockplus.org/29922555/diff/29922556/README.md#newcode95 README.md:95: clients to update their lists more frequently using less resources. We normally try to limit line length to <80 characters in the documentation files. It's not really documented anywhere but it's pretty consistent and it's easier to read such files so I would recommend to format this section this way. https://codereview.adblockplus.org/29922555/diff/29922556/README.md#newcode97 README.md:97: Python-abp contains a script called `fldiff` that will find the diff between the lastest Nit: should it be "latest"? https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:23: import os.path You can actually just `import os` here and `os.path.join` and other functions will be available the same way. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:37: parser.add_argument( 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') https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:39: default='-', nargs=1) Since -o is now a directory, it can't default to `stdout`. Probably current directory is a reasonable default. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:43: return parser.parse_known_args(args_in) 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 https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:52: raise MissingFilterListError('The --latest list is required') 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? https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:57: raise MissingFilterListError('Archived lists are required') This should also be handled by the arguments parser. You can use `nargs='+'` to make a positional argument multivalued and requiring at least one. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:60: out_dir = arg[0].output_dir[0].strip() 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. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:67: version = re.search(r'.*(\d+)\.txt', base_file).groups()[0] 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? https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:69: 'diff{}.txt'.format(version)) The format of the output filename should probably be configurable. How about adding a '-f FORMAT' argument with the default that you have here? https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:41: def archive_dir(tmpdir, rootdir): You don't seem to be using tmpdir in this function. Probably no need to depend on it. https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:50: def diff_dir(tmpdir, rootdir): tmpdir also unused here, and the function could be made a bit more compact: return rootdir.mkdir('diff') https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:69: stdout, stderr = proc.communicate() Since we can't really output to stdout anymore (because we're producing multiple files), we can probably switch to using subprocess.run() here. https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:77: args.append(str(archive_dir.join('list1.txt'))) With the way we use `archive_dir` fixture, I'm thinking that perhaps it would be better if it would be called `archived_files` and would return the list of paths. Then we would just do `args.extend(archived_files)` here and in the other test. Two advantages of this approach are not duplicating the code that produces the paths in several tests (we also have part of it in the fixture) and avoiding hidden dependency on the list of files in the directory (any test that uses the fixture also needs to have the knowledge what files are in the directory: the fixture could just contain this information instead). https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:104: def test_no_outfile(rootdir, archive_dir, diff_dir): Since this mode of running doesn't really work with multiple base files, I think it's the easiest to just stop supporting it (see also comments in the script). We would not need this test then.
Hey Vasily! Thanks for the feedback. One quick note: at some point I can change all the `arg` variables back to `args`, but for now, pdb gets confused when I try to examine `args`, and it only gives me the arguments to the current function, so I'm using `arg` instead. Let me know if you have any other issues or suggestions? https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:23: import os.path On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > You can actually just `import os` here and `os.path.join` and other functions > will be available the same way. Done. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:37: parser.add_argument( On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > 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') Done. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:39: default='-', nargs=1) On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > Since -o is now a directory, it can't default to `stdout`. Probably current > directory is a reasonable default. 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. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:43: return parser.parse_known_args(args_in) On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > 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 Done. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:52: raise MissingFilterListError('The --latest list is required') On 2018/10/24 11:30:08, Vasily Kuznetsov wrote: > 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? Yeah, using a positional argument makes sense, and the usage looks nicer. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:57: raise MissingFilterListError('Archived lists are required') On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > This should also be handled by the arguments parser. You can use `nargs='+'` to > make a positional argument multivalued and requiring at least one. Done. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:60: out_dir = arg[0].output_dir[0].strip() On 2018/10/24 11:30:08, Vasily Kuznetsov wrote: > 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. 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. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:67: version = re.search(r'.*(\d+)\.txt', base_file).groups()[0] On 2018/10/24 11:30:08, Vasily Kuznetsov wrote: > 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? Yeah, I wasn't sure how reliable this would be. Done. https://codereview.adblockplus.org/29922555/diff/29922556/abp/filters/diff_sc... abp/filters/diff_script.py:69: 'diff{}.txt'.format(version)) On 2018/10/24 11:30:08, Vasily Kuznetsov wrote: > The format of the output filename should probably be configurable. How about > adding a '-f FORMAT' argument with the default that you have here? 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? https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:41: def archive_dir(tmpdir, rootdir): On 2018/10/24 11:30:10, Vasily Kuznetsov wrote: > You don't seem to be using tmpdir in this function. Probably no need to depend > on it. Done. https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:50: def diff_dir(tmpdir, rootdir): On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > tmpdir also unused here, and the function could be made a bit more compact: > > return rootdir.mkdir('diff') Done. https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:69: stdout, stderr = proc.communicate() On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > Since we can't really output to stdout anymore (because we're producing multiple > files), we can probably switch to using subprocess.run() here. Looks like subprocess.run() was added in Python 3.5, and won't be compatible with the Python 2.7 tests [https://docs.python.org/3/library/subprocess.html]. The documentation recommends using subprocess.call() or .check_call() to remain backwards-compatible. Unless we want to stop supporting Python 2? https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:77: args.append(str(archive_dir.join('list1.txt'))) On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > With the way we use `archive_dir` fixture, I'm thinking that perhaps it would be > better if it would be called `archived_files` and would return the list of > paths. Then we would just do `args.extend(archived_files)` here and in the other > test. > > Two advantages of this approach are not duplicating the code that produces the > paths in several tests (we also have part of it in the fixture) and avoiding > hidden dependency on the list of files in the directory (any test that uses the > fixture also needs to have the knowledge what files are in the directory: the > fixture could just contain this information instead). Yeah, this way seems much cleaner. https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:104: def test_no_outfile(rootdir, archive_dir, diff_dir): On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > Since this mode of running doesn't really work with multiple base files, I think > it's the easiest to just stop supporting it (see also comments in the script). > We would not need this test then. Done.
Hi Rose! Great progress, this is almost done! I think we also need a test for absent `-o` (output to the current directory) and maybe one for insufficient number of arguments (like `fldiff base.txt`). I also put a couple more suggestions in the comments below. Cheers, Vasily https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29922556/tests/test_diff_scr... tests/test_diff_script.py:69: stdout, stderr = proc.communicate() On 2018/10/26 21:42:22, rhowell wrote: > On 2018/10/24 11:30:09, Vasily Kuznetsov wrote: > > Since we can't really output to stdout anymore (because we're producing > multiple > > files), we can probably switch to using subprocess.run() here. > > Looks like subprocess.run() was added in Python 3.5, and won't be compatible > with the Python 2.7 tests [https://docs.python.org/3/library/subprocess.html]. > The documentation recommends using subprocess.call() or .check_call() to remain > backwards-compatible. Unless we want to stop supporting Python 2? Indeed, sorry, I forgot that we're also supporting Python 2 with this :/ https://codereview.adblockplus.org/29922555/diff/29928573/abp/filters/diff_sc... File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29928573/abp/filters/diff_sc... abp/filters/diff_script.py:41: raise MissingFilterListError('Unable to find Version in [{}]'.format(base)) Wouldn't it be more helpful to give filename in this error message instead of the version from the header? What do you think? https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:77: return [str(rootdir.join('latest.txt')), '-o ' + str(diff_dir)] '-o' and str(diff_dir) would be different arguments, normally, when the script is called from a shell. It seems that this works anyway, but I think it would be more realistic to replace it with ", '-o', str(diff_dir)]". https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:81: arg = _make_args(rootdir, diff_dir) I wonder if _make_args is actually worth it. Here if we don't call it, we get something like: diff_script([str(rootdir.join('latest.txt')), '-o', str(diff_dir)] + archived_files) Maybe it's even easier to understand. What do you think? https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:96: run_script(*arg) Following on the previous comment, here we'd get something like: run_script(str(rootdir.join('latest.txt')), '-o', str(diff_dir), *archived_files) What do you think about this version?
Hey Vasily! I added the test with no outfile specified, good idea. I already had a test that covers the case where not enough files are given, `test_no_base_file()`, do you think that is sufficient? It runs `fldiff latest.txt`. Maybe I'll rename the test so it's clearer, so `test_not_enough_args()`. Also, Did we want to make the outputted filenames configurable? Maybe `[pre][version].txt` or `[pre][version][post]`? https://codereview.adblockplus.org/29922555/diff/29928573/abp/filters/diff_sc... File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29928573/abp/filters/diff_sc... abp/filters/diff_script.py:41: raise MissingFilterListError('Unable to find Version in [{}]'.format(base)) On 2018/10/29 16:31:48, Vasily Kuznetsov wrote: > Wouldn't it be more helpful to give filename in this error message instead of > the version from the header? What do you think? Yeah. With this (old) method, it returns the header line: Unable to find Version in [Adblock Plus 2.0] But it does seem more accurate and explicit to return the file's name: Unable to find Version in /tmp/pytest-of-emma/pytest-372/test_no_version0/root/archive/list113.txt https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:77: return [str(rootdir.join('latest.txt')), '-o ' + str(diff_dir)] On 2018/10/29 16:31:48, Vasily Kuznetsov wrote: > '-o' and str(diff_dir) would be different arguments, normally, when the script > is called from a shell. It seems that this works anyway, but I think it would be > more realistic to replace it with ", '-o', str(diff_dir)]". Ended up just removing this function, but I did switch to this style in the other functions. https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:81: arg = _make_args(rootdir, diff_dir) On 2018/10/29 16:31:48, Vasily Kuznetsov wrote: > I wonder if _make_args is actually worth it. Here if we don't call it, we get > something like: > > diff_script([str(rootdir.join('latest.txt')), '-o', str(diff_dir)] > + archived_files) > > Maybe it's even easier to understand. What do you think? Done. https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:96: run_script(*arg) On 2018/10/29 16:31:48, Vasily Kuznetsov wrote: > Following on the previous comment, here we'd get something like: > > run_script(str(rootdir.join('latest.txt')), '-o', str(diff_dir), > *archived_files) > > What do you think about this version? Done.
Hi Rosie! Pretty good. There are a couple small nits in the comments below, and am I right in understanding that you want to remove test_diff_with_dict() and some related code in diff_script.py? If so, I think we can probably do it already. Or what was the idea? Cheers, Vasily https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29928573/tests/test_diff_scr... tests/test_diff_script.py:77: return [str(rootdir.join('latest.txt')), '-o ' + str(diff_dir)] On 2018/10/29 20:01:40, rhowell wrote: > On 2018/10/29 16:31:48, Vasily Kuznetsov wrote: > > '-o' and str(diff_dir) would be different arguments, normally, when the script > > is called from a shell. It seems that this works anyway, but I think it would > be > > more realistic to replace it with ", '-o', str(diff_dir)]". > > Ended up just removing this function, but I did switch to this style in the > other functions. Acknowledged. https://codereview.adblockplus.org/29922555/diff/29931193/abp/filters/diff_sc... File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931193/abp/filters/diff_sc... abp/filters/diff_script.py:35: def _get_version(filterlist, base): Nit: maybe this argument can just be called "filename" here, since this is what it means basically. https://codereview.adblockplus.org/29922555/diff/29931193/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931193/tests/test_diff_scr... tests/test_diff_script.py:79: for _, _, files in os.walk(str(diff_dir)): I just realized that we don't really need `os.walk` here. A simple `glob.glob` or perhaps `diff_dir.visit()` [1] would suffice. [1] https://py.readthedocs.io/en/latest/path.html#py._path.local.LocalPath.visit https://codereview.adblockplus.org/29922555/diff/29931193/tests/test_diff_scr... tests/test_diff_script.py:102: run_script(str(rootdir.join('latest.txt')), *archived_files) We need to change to a temporary directory before doing this. We might not have write access to the directory where the tests are run. Also, it's cleaner to write to a temporary directory instead of removing the files afterwards.
Hey Vasily! I removed the test with the dictionary and the related code, and added a test to make sure we can write and over-write to the diff files. Oh, and did we want to make the diff filename format configurable? I checked with Paco in #iflu, he doesn't mind either way. Thanks for the feedback, and let me know if you see anything else? https://codereview.adblockplus.org/29922555/diff/29931193/abp/filters/diff_sc... File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931193/abp/filters/diff_sc... abp/filters/diff_script.py:35: def _get_version(filterlist, base): On 2018/10/29 23:47:33, Vasily Kuznetsov wrote: > Nit: maybe this argument can just be called "filename" here, since this is what > it means basically. Done. https://codereview.adblockplus.org/29922555/diff/29931193/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931193/tests/test_diff_scr... tests/test_diff_script.py:79: for _, _, files in os.walk(str(diff_dir)): On 2018/10/29 23:47:34, Vasily Kuznetsov wrote: > I just realized that we don't really need `os.walk` here. A simple `glob.glob` > or perhaps `diff_dir.visit()` [1] would suffice. > > [1] https://py.readthedocs.io/en/latest/path.html#py._path.local.LocalPath.visit Yeah, this seems to be less code and more efficient. Done. https://codereview.adblockplus.org/29922555/diff/29931193/tests/test_diff_scr... tests/test_diff_script.py:102: run_script(str(rootdir.join('latest.txt')), *archived_files) On 2018/10/29 23:47:34, Vasily Kuznetsov wrote: > We need to change to a temporary directory before doing this. We might not have > write access to the directory where the tests are run. Also, it's cleaner to > write to a temporary directory instead of removing the files afterwards. Done.
Hi Rosie! Just a couple more small things. Cheers, Vasily https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_scr... tests/test_diff_script.py:31: """Root directory holds the latest filter list, and folders for archived Multiline docstrings should "consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description" accodring to PEP257 [1], which is recommended by our style guide. In this case I think there are two approaches to make this docstring compatible: either reformulate it and make it short enough to fit into one line or do something like: """Root directory for test files. Yada yada yada. """ [1]: https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_scr... tests/test_diff_script.py:79: assert len(os.listdir(str(diff_dir))) == 2 It's a good idea to check how many files were created but we should probably be consistent between this line and the next. I'd be happy with either approach.
On 2018/10/31 00:20:59, rhowell wrote: > > Oh, and did we want to make the diff filename format configurable? I checked > with Paco in #iflu, he doesn't mind either way. > I think non-configurable is ok for now, we can just make a separate ticket if we need configurability.
Thanks for the feedback, Vasily! Sounds good about the configurability, if that feature is wanted in the future. Let me know if there's any other suggestions. :) https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_scr... File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_scr... tests/test_diff_script.py:31: """Root directory holds the latest filter list, and folders for archived On 2018/10/31 14:42:39, Vasily Kuznetsov wrote: > Multiline docstrings should "consist of a summary line just like a one-line > docstring, followed by a blank line, followed by a more elaborate description" > accodring to PEP257 [1], which is recommended by our style guide. In this case I > think there are two approaches to make this docstring compatible: either > reformulate it and make it short enough to fit into one line or do something > like: > > """Root directory for test files. > > Yada yada yada. > """ > > [1]: https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings Oops, thanks! I knew the multiline docstring looked weird, but I forgot it's in the style guide. I shortened it; hopefully the code is clear enough so we don't need as much documentation. https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_scr... tests/test_diff_script.py:79: assert len(os.listdir(str(diff_dir))) == 2 On 2018/10/31 14:42:39, Vasily Kuznetsov wrote: > It's a good idea to check how many files were created but we should probably be > consistent between this line and the next. I'd be happy with either approach. Yeah, it seems more consistent to use the functions available from the LocalPath object. My main concern here was that, if the diff_dir is empty, these tests would pass, unless we verify that some files are actually there.
Hi Rosie! LGTM from my side. Maybe get Paco to look at the command line API and check if he's happy with what we have now. Cheers, Vasily P.S. It seems that the latest version of flake8 started giving errors in python-abp but it doesn't seem like this is your fault so we should address this in a separate ticket.
> Maybe get Paco to look at the command line API and check if he's happy with what > we have now. He said it looks good. :) > P.S. It seems that the latest version of flake8 started giving errors in > python-abp but it doesn't seem like this is your fault so we should address this > in a separate ticket. You saw that too? I thought it was just me! Haven't had a chance to look into it yet. But, I think it should be fine to push this for now anyway. I'll go push it now. :) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
