Issue 29400555: Issue 4992 - Adds optional revision arg to generate_static_pages (Closed)
|
Created:
April 1, 2017, 12:21 p.m. by Jon Sonesen Modified:
April 24, 2017, 7:15 a.m. Base URL:
https://hg.adblockplus.org/cms Visibility:
Public. |
DescriptionIssue 4992 - Adds optional revision arg to generate_static_pages
Patch Set 1 : #
Total comments: 13
Patch Set 2 : add test coverage for static generation without the optional argument '-r/--rev' #Patch Set 3 : address comments for argparse descriptions #
Total comments: 7
Patch Set 4 : removes duplication in fixtures and addresses arg message changes #
Total comments: 4
Patch Set 5 : fixed redundant sys.argv definition #
MessagesTotal messages: 16
Here is the functional patch. So far I have tested generating the web.eyeo.com site in addition to the tests I wrote. The tests, should probably be refactored to accommodate the additional options, but I thought we may want to discuss what to parametrize further or even is it should be a separate test module. https://codereview.adblockplus.org/29400555/diff/29400568/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/tests/test_page_out... tests/test_page_outputs.py:65: def test_revision_arg(output_pages): The bookmark consists of an additional page 'bar' which was added in the second commit. The master bookmark should not contain this.
Hey Jon, Looks good. I think we could make the help messages a bit more helpful and we should also cover the case with no '-r' option in the tests (see below). Cheers, Vasily https://codereview.adblockplus.org/29400555/diff/29400568/cms/bin/generate_st... File cms/bin/generate_static_pages.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/cms/bin/generate_st... cms/bin/generate_static_pages.py:147: parser = ArgumentParser() Perhaps we should add a description here to make the help output more friendly. For example "Convert website source to a static website". https://codereview.adblockplus.org/29400555/diff/29400568/cms/bin/generate_st... cms/bin/generate_static_pages.py:149: help='see "hg help revisions" for more details', It's nice that you refer to "hg help revisions", but I think we should also explain briefly here what -r option means. Something like "revision of the source repository to use for generation". https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py#newc... cms/sources.py:211: def __init__(self, repo, revision): Perhaps add a default value 'default' in case someone uses this from another script. https://codereview.adblockplus.org/29400555/diff/29400568/tests/conftest.py File tests/conftest.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/tests/conftest.py#n... tests/conftest.py:20: subprocess.check_call(['hg', '-R', site_dir, 'bookmark', 'test']) Are we actually using this second bookmark? https://codereview.adblockplus.org/29400555/diff/29400568/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/tests/test_page_out... tests/test_page_outputs.py:65: def test_revision_arg(output_pages): We should also run generation with no '--rev' argument and make sure that works correctly. One possible way to do it would be to not add '--rev', 'master' as default arguments in the `static_output` fixture and only add them in this test. Then we could add an empty expected output for `bar.md` using the standard approach (and normal testing will check that it's generated) and in this test we would check that `bar` is not generated. Does this make sense?
https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py#newc... cms/sources.py:211: def __init__(self, repo, revision): On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > Perhaps add a default value 'default' in case someone uses this from another > script. It is not used anywhere else, as verified by grep in the repository which is why I left it this way. Is there a strong argument one way or another? https://codereview.adblockplus.org/29400555/diff/29400568/tests/conftest.py File tests/conftest.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/tests/conftest.py#n... tests/conftest.py:20: subprocess.check_call(['hg', '-R', site_dir, 'bookmark', 'test']) On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > Are we actually using this second bookmark? yes, it is what we use to commit the 'bar.md' file to the default branch and then we test that 'master' bokmark doesnt hold the file that was added in the 'test' bookmark. https://codereview.adblockplus.org/29400555/diff/29400568/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/tests/test_page_out... tests/test_page_outputs.py:65: def test_revision_arg(output_pages): On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > We should also run generation with no '--rev' argument and make sure that works > correctly. One possible way to do it would be to not add '--rev', 'master' as > default arguments in the `static_output` fixture and only add them in this test. > Then we could add an empty expected output for `bar.md` using the standard > approach (and normal testing will check that it's generated) and in this test we > would check that `bar` is not generated. Does this make sense? Acknowledged.
https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py#newc... cms/sources.py:211: def __init__(self, repo, revision): On 2017/04/03 10:10:59, Jon Sonesen wrote: > On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > > Perhaps add a default value 'default' in case someone uses this from another > > script. > > It is not used anywhere else, as verified by grep in the repository which is why > I left it this way. Is there a strong argument one way or another? That is the same conclusion to which I came. When I saw these changes, I thought we should specify a default value here. But then I noticed that defaults are already handled by argsparse, so that any default would be redundant here, and as we don't use this class anywhere else this is unnecessary.
https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py File cms/sources.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/cms/sources.py#newc... cms/sources.py:211: def __init__(self, repo, revision): On 2017/04/03 10:18:49, Sebastian Noack wrote: > On 2017/04/03 10:10:59, Jon Sonesen wrote: > > On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > > > Perhaps add a default value 'default' in case someone uses this from another > > > script. > > > > It is not used anywhere else, as verified by grep in the repository which is > why > > I left it this way. Is there a strong argument one way or another? > > That is the same conclusion to which I came. When I saw these changes, I thought > we should specify a default value here. But then I noticed that defaults are > already handled by argsparse, so that any default would be redundant here, and > as we don't use this class anywhere else this is unnecessary. I was thinking about possible future uses, but it's not a big deal, just would be a slightly nicer API. Anyway, since this is unlikely to ever be used by any code not inside CMS, I don't think it's that important. Can also just stay as is.
So then we can leave the revision default in argparse? https://codereview.adblockplus.org/29400555/diff/29400568/cms/bin/generate_st... File cms/bin/generate_static_pages.py (right): https://codereview.adblockplus.org/29400555/diff/29400568/cms/bin/generate_st... cms/bin/generate_static_pages.py:147: parser = ArgumentParser() On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > Perhaps we should add a description here to make the help output more friendly. > For example "Convert website source to a static website". Done. https://codereview.adblockplus.org/29400555/diff/29400568/cms/bin/generate_st... cms/bin/generate_static_pages.py:149: help='see "hg help revisions" for more details', On 2017/04/03 09:55:08, Vasily Kuznetsov wrote: > It's nice that you refer to "hg help revisions", but I think we should also > explain briefly here what -r option means. Something like "revision of the > source repository to use for generation". Done.
https://codereview.adblockplus.org/29400555/diff/29401580/cms/bin/generate_st... File cms/bin/generate_static_pages.py (right): https://codereview.adblockplus.org/29400555/diff/29401580/cms/bin/generate_st... cms/bin/generate_static_pages.py:150: 'see "hg help revisions" for info', default='default') Maybe put this in parentheses? Otherwise we just glue two sentences together with only a space in between. https://codereview.adblockplus.org/29400555/diff/29401580/cms/bin/generate_st... cms/bin/generate_static_pages.py:151: parser.add_argument('source', help="Path to your website's repository") I kind of feel that "your" is not quite appropriate in the option description. It sounds kind of too informal or something. Maybe "Path to website's repository" would be better? What do you think? https://codereview.adblockplus.org/29400555/diff/29401580/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29401580/tests/test_page_out... tests/test_page_outputs.py:31: sys.argv = ['filler', temp_site, static_out_path+'master', I was actually thinking to only generate the master bookmark for `test_revision_arg` and leave everything else as is. It seems like it would be simpler than generating two websites. Or if you want to run all the tests on both, maybe we could just use parametrize somehow?
On 2017/04/03 10:53:52, Jon Sonesen wrote: > So then we can leave the revision default in argparse? Yeah, that one we should definitely leave in any case.
https://codereview.adblockplus.org/29400555/diff/29401580/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29401580/tests/test_page_out... tests/test_page_outputs.py:31: sys.argv = ['filler', temp_site, static_out_path+'master', On 2017/04/03 13:02:19, Vasily Kuznetsov wrote: > I was actually thinking to only generate the master bookmark for > `test_revision_arg` and leave everything else as is. It seems like it would be > simpler than generating two websites. Or if you want to run all the tests on > both, maybe we could just use parametrize somehow? I am not sure how this would work then. The thing is that want to test that the generation is done with the expected, provided revision. So unless we only wanted to test that the argparse code works then wouldn't we have to generate two sites to ensure they are not from the same revision/bookmark and to test this wouldn't we need to add a file or some other change in the generated site? Perhaps I am misunderstanding how you want to test the behavior.
https://codereview.adblockplus.org/29400555/diff/29401580/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29401580/tests/test_page_out... tests/test_page_outputs.py:31: sys.argv = ['filler', temp_site, static_out_path+'master', On 2017/04/03 13:02:19, Vasily Kuznetsov wrote: > I was actually thinking to only generate the master bookmark for > `test_revision_arg` and leave everything else as is. It seems like it would be > simpler than generating two websites. Or if you want to run all the tests on > both, maybe we could just use parametrize somehow? As discussed over Mumble we agree this bit of duplication in tests and everything is not needed. Instead I will alter this method to accept a new parametrized fixture called 'revision' which returns None, and 'master' then based on this value we can assert the expected outputs.
https://codereview.adblockplus.org/29400555/diff/29401580/cms/bin/generate_st... File cms/bin/generate_static_pages.py (right): https://codereview.adblockplus.org/29400555/diff/29401580/cms/bin/generate_st... cms/bin/generate_static_pages.py:150: 'see "hg help revisions" for info', default='default') On 2017/04/03 13:02:19, Vasily Kuznetsov wrote: > Maybe put this in parentheses? Otherwise we just glue two sentences together > with only a space in between. Done. https://codereview.adblockplus.org/29400555/diff/29401580/cms/bin/generate_st... cms/bin/generate_static_pages.py:151: parser.add_argument('source', help="Path to your website's repository") On 2017/04/03 13:02:19, Vasily Kuznetsov wrote: > I kind of feel that "your" is not quite appropriate in the option description. > It sounds kind of too informal or something. Maybe "Path to website's > repository" would be better? What do you think? Done.
Almost done. Just a small correction to the fixture. https://codereview.adblockplus.org/29400555/diff/29401601/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29401601/tests/test_page_out... tests/test_page_outputs.py:36: if revision is None: We don't really need to repeat the standard args twice: sys.argv = ['filler', temp_site, static_out_path] if revision is not None: sys.argv += ['--rev', revision] https://codereview.adblockplus.org/29400555/diff/29401601/tests/test_page_out... tests/test_page_outputs.py:39: sys.argv = ['filler', temp_site, static_out_path, '--rev', 'master'] The last item of the list should be `revision` but see comment above.
https://codereview.adblockplus.org/29400555/diff/29401601/tests/test_page_out... File tests/test_page_outputs.py (right): https://codereview.adblockplus.org/29400555/diff/29401601/tests/test_page_out... tests/test_page_outputs.py:36: if revision is None: On 2017/04/03 15:15:52, Vasily Kuznetsov wrote: > We don't really need to repeat the standard args twice: > > sys.argv = ['filler', temp_site, static_out_path] > if revision is not None: > sys.argv += ['--rev', revision] Done. https://codereview.adblockplus.org/29400555/diff/29401601/tests/test_page_out... tests/test_page_outputs.py:39: sys.argv = ['filler', temp_site, static_out_path, '--rev', 'master'] On 2017/04/03 15:15:52, Vasily Kuznetsov wrote: > The last item of the list should be `revision` but see comment above. Done.
LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
