Issue 5431 - Fix test for copyright update script

Issue 5431 - Fix test for copyright update script

[Vasily Kuznetsov, 2017-07-20 15:01:30]

Looks good. I have a couple of suggestions:

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
File update-copyright/tests/test_update_copyright.py (right):

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:18: def generate_sample_file():
It would be a bit more clear to call the fixture `sample_file` and to make it
return the path. This way we make the dependency on the file more explicit and
remove duplication of the code that constructs this path.

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:20: text = '# {}right (C)
2006-2015 eyeo GmbH\n'.format('Copy')
If is important to have different years in different instances of the copyright
thingie? If not, perhaps you could first generate the whole copyright
construction and then just reuse it. Something like:

    cr = 'Copy' + 'right (C) ....'
    text = '# {}\nvalue = '{}'\n# {}'.format(cr, cr, cr)

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:23: sample_file =
open(os.path.join(data_path, 'sample_file.py'), 'w')
Would it work to create this file in a temporary directory (`tmpdir`) instead?
That would be a bit cleaner and it would get autocleaned by pytest.

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:23: sample_file =
open(os.path.join(data_path, 'sample_file.py'), 'w')
You could use `with` here instead, then you don't need an explicit `close`:

    with open(...) as sample_file:
        sample_file.write(...)

[rosie, 2017-08-03 17:52:46]

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
File update-copyright/tests/test_update_copyright.py (right):

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:18: def generate_sample_file():
On 2017/07/20 15:01:30, Vasily Kuznetsov wrote:
> It would be a bit more clear to call the fixture `sample_file` and to make it
> return the path. This way we make the dependency on the file more explicit and
> remove duplication of the code that constructs this path.

Done.

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:20: text = '# {}right (C)
2006-2015 eyeo GmbH\n'.format('Copy')
On 2017/07/20 15:01:30, Vasily Kuznetsov wrote:
> If is important to have different years in different instances of the
copyright
> thingie? If not, perhaps you could first generate the whole copyright
> construction and then just reuse it. Something like:
> 
>     cr = 'Copy' + 'right (C) ....'
>     text = '# {}\nvalue = '{}'\n# {}'.format(cr, cr, cr)

The third line uses 'example GmbH' instead of 'eyeo GmbH', to test for false
positives. I could format the first two lines as you suggest, but it would be
the same number of lines of code in the end. The current structure is a bit
easier for me to read, but I don't feel strongly about it either way. If you
think it's cleaner to use the one you suggest, I can change it.

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:23: sample_file =
open(os.path.join(data_path, 'sample_file.py'), 'w')
On 2017/07/20 15:01:30, Vasily Kuznetsov wrote:
> You could use `with` here instead, then you don't need an explicit `close`:
> 
>     with open(...) as sample_file:
>         sample_file.write(...)

Done.

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:23: sample_file =
open(os.path.join(data_path, 'sample_file.py'), 'w')
On 2017/07/20 15:01:30, Vasily Kuznetsov wrote:
> Would it work to create this file in a temporary directory (`tmpdir`) instead?
> That would be a bit cleaner and it would get autocleaned by pytest.

Done.

[rosie, 2017-08-03 18:24:17]

Please ignore Patch Sets 2 & 3. I was submitting the wrong revision number, but
thought it was an issue with the typo in patchconv/README.md. If you compare
Patch Sets 1 & 4, those should be the changes I meant to push. Sorry!

[Vasily Kuznetsov, 2017-08-04 16:54:47]

Hi Rose,

Looks good. I just have one suggestion and one question.

Cheers,
Vasily

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
File update-copyright/tests/test_update_copyright.py (right):

https://codereview.adblockplus.org/29493601/diff/29493602/update-copyright/te...
update-copyright/tests/test_update_copyright.py:20: text = '# {}right (C)
2006-2015 eyeo GmbH\n'.format('Copy')
On 2017/08/03 17:52:45, rosie wrote:
> On 2017/07/20 15:01:30, Vasily Kuznetsov wrote:
> > If is important to have different years in different instances of the
> copyright
> > thingie? If not, perhaps you could first generate the whole copyright
> > construction and then just reuse it. Something like:
> > 
> >     cr = 'Copy' + 'right (C) ....'
> >     text = '# {}\nvalue = '{}'\n# {}'.format(cr, cr, cr)
> 
> The third line uses 'example GmbH' instead of 'eyeo GmbH', to test for false
> positives. I could format the first two lines as you suggest, but it would be
> the same number of lines of code in the end. The current structure is a bit
> easier for me to read, but I don't feel strongly about it either way. If you
> think it's cleaner to use the one you suggest, I can change it.

Acknowledged.

https://codereview.adblockplus.org/29493601/diff/29504604/patchconv/README.md
File patchconv/README.md (right):

https://codereview.adblockplus.org/29493601/diff/29504604/patchconv/README.md...
patchconv/README.md:20: $ pip install
'hg+https://hg.[Dadblockplus.org/codingtools#egg=patchconv&subdirectory=patchconv'
This seems like an accidental change. Or am I missing something?

https://codereview.adblockplus.org/29493601/diff/29504604/update-copyright/te...
File update-copyright/tests/test_update_copyright.py (right):

https://codereview.adblockplus.org/29493601/diff/29504604/update-copyright/te...
update-copyright/tests/test_update_copyright.py:23: with open(str(sample_file),
'w') as sample_file_path:
Since now `sample_file` is a `py.path` object, you can just do
`sample_file.write(text)`.

https://codereview.adblockplus.org/29493601/diff/29504604/update-copyright/te...
update-copyright/tests/test_update_copyright.py:115: shutil.copy(sample_file,
os.path.join(base_dir, 'repo_1'))
Isn't the sample file already copied to the repositories when they are created
(line 33)?

[rosie, 2017-08-05 18:41:08]

https://codereview.adblockplus.org/29493601/diff/29504604/patchconv/README.md
File patchconv/README.md (right):

https://codereview.adblockplus.org/29493601/diff/29504604/patchconv/README.md...
patchconv/README.md:20: $ pip install
'hg+https://hg.[Dadblockplus.org/codingtools#egg=patchconv&subdirectory=patchconv'
On 2017/08/04 16:54:46, Vasily Kuznetsov wrote:
> This seems like an accidental change. Or am I missing something?

Yes. Must've happened when I was working on the update_copyright README; I was
looking at how other README's were formatted for examples. Can we discard this
change, or should I change it back?

https://codereview.adblockplus.org/29493601/diff/29504604/update-copyright/te...
File update-copyright/tests/test_update_copyright.py (right):

https://codereview.adblockplus.org/29493601/diff/29504604/update-copyright/te...
update-copyright/tests/test_update_copyright.py:23: with open(str(sample_file),
'w') as sample_file_path:
On 2017/08/04 16:54:46, Vasily Kuznetsov wrote:
> Since now `sample_file` is a `py.path` object, you can just do
> `sample_file.write(text)`.

Done.

https://codereview.adblockplus.org/29493601/diff/29504604/update-copyright/te...
update-copyright/tests/test_update_copyright.py:115: shutil.copy(sample_file,
os.path.join(base_dir, 'repo_1'))
On 2017/08/04 16:54:46, Vasily Kuznetsov wrote:
> Isn't the sample file already copied to the repositories when they are created
> (line 33)?

Done.

[Vasily Kuznetsov, 2017-08-06 10:04:06]

LGTM

[Sebastian Noack, 2017-08-06 13:10:32]

LGTM