Noissue - Add a script for converting Rietveld patches to Git format

Noissue - Add a script for converting Rietveld patches to Git format

[Vasily Kuznetsov, 2016-05-12 17:56:35]

With this converter we can apply patches from reviews even if they have moves,
copies and binary files. This makes it easier to preview the patch locally,
which is sometimes more convenient than looking through Rietveld.

[Sebastian Noack, 2016-05-12 21:44:27]

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv.py
File patchconv/patchconv.py (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv...
patchconv/patchconv.py:74: def script():
This seems to be an unconventional name for an entry point function, or is it
indeed common? As far as I can tell people usually call it main()?

Also I wonder whether it makes quickly testing changes unnecessarily complicated
since you cannot run this module as script without installing it first.

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini
File patchconv/tox.ini (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini#n...
patchconv/tox.ini:10:
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
Assuming this code goes into the coding tools repo as well, you could simply
refer to ../flake-abp here I think.

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini#n...
patchconv/tox.ini:10:
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
What's about pep8-naming?

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini#n...
patchconv/tox.ini:14: flake8 patchconv.py
Please also run flake8 on tests and setup.py.

[Vasily Kuznetsov, 2016-05-12 23:25:16]

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv.py
File patchconv/patchconv.py (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv...
patchconv/patchconv.py:74: def script():
On 2016/05/12 21:44:26, Sebastian Noack wrote:
> This seems to be an unconventional name for an entry point function, or is it
> indeed common? As far as I can tell people usually call it main()?
> 
> Also I wonder whether it makes quickly testing changes unnecessarily
complicated
> since you cannot run this module as script without installing it first.

Quickly testing is still pretty easy after `setup.py develop` but you're
probably right that `main` is a more conventional name. Let's make it main().

Done.

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini
File patchconv/tox.ini (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini#n...
patchconv/tox.ini:10:
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
On 2016/05/12 21:44:26, Sebastian Noack wrote:
> Assuming this code goes into the coding tools repo as well, you could simply
> refer to ../flake-abp here I think.

Seems to work. Ok, that's simpler.

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini#n...
patchconv/tox.ini:10:
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
On 2016/05/12 21:44:27, Sebastian Noack wrote:
> What's about pep8-naming?

Added.

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/tox.ini#n...
patchconv/tox.ini:14: flake8 patchconv.py
On 2016/05/12 21:44:27, Sebastian Noack wrote:
> Please also run flake8 on tests and setup.py.

Oops, somehow that fell through the cracks.

Done.

[Sebastian Noack, 2016-05-12 23:48:52]

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv.py
File patchconv/patchconv.py (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv...
patchconv/patchconv.py:74: def script():
On 2016/05/12 23:25:16, Vasily Kuznetsov wrote:
> On 2016/05/12 21:44:26, Sebastian Noack wrote:
> > This seems to be an unconventional name for an entry point function, or is
it
> > indeed common? As far as I can tell people usually call it main()?
> > 
> > Also I wonder whether it makes quickly testing changes unnecessarily
> complicated
> > since you cannot run this module as script without installing it first.
> 
> Quickly testing is still pretty easy after `setup.py develop` but you're
> probably right that `main` is a more conventional name. Let's make it main().
> 
> Done.

I'm just curious is this common practice? Most Python scripts like this I've
seen so far use if __name__ == '__main__'

https://codereview.adblockplus.org/29341347/diff/29341363/patchconv/tox.ini
File patchconv/tox.ini (right):

https://codereview.adblockplus.org/29341347/diff/29341363/patchconv/tox.ini#n...
patchconv/tox.ini:15: flake8 patchconv.py setup.py tests/test_rietveld_to_git.py
Note that flake8 can automatically find Python files. So instead hardcoding 3
files here, you could just use --exclude=build

[Vasily Kuznetsov, 2016-05-17 15:57:31]

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv.py
File patchconv/patchconv.py (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv...
patchconv/patchconv.py:74: def script():
On 2016/05/12 23:48:51, Sebastian Noack wrote:
> On 2016/05/12 23:25:16, Vasily Kuznetsov wrote:
> > On 2016/05/12 21:44:26, Sebastian Noack wrote:
> > > This seems to be an unconventional name for an entry point function, or is
> it
> > > indeed common? As far as I can tell people usually call it main()?
> > > 
> > > Also I wonder whether it makes quickly testing changes unnecessarily
> > complicated
> > > since you cannot run this module as script without installing it first.
> > 
> > Quickly testing is still pretty easy after `setup.py develop` but you're
> > probably right that `main` is a more conventional name. Let's make it
main().
> > 
> > Done.
> 
> I'm just curious is this common practice? Most Python scripts like this I've
> seen so far use if __name__ == '__main__'

if __name__ == '__main__' allows running the script with `python script.py` or
`python -m package.script`. However, this script is installed via
`console_scripts` entry point in setup.py, so it can be run simply by typing
`patchconv`. With this in mind it seems to me that the other ways of running it
are a bit redundant. If we want more flexibility, I could add `if __name__ ==
'__main__': main()` to the end of this file to enable the other options. Do you
think it would be useful?

https://codereview.adblockplus.org/29341347/diff/29341363/patchconv/tox.ini
File patchconv/tox.ini (right):

https://codereview.adblockplus.org/29341347/diff/29341363/patchconv/tox.ini#n...
patchconv/tox.ini:15: flake8 patchconv.py setup.py tests/test_rietveld_to_git.py
On 2016/05/12 23:48:51, Sebastian Noack wrote:
> Note that flake8 can automatically find Python files. So instead hardcoding 3
> files here, you could just use --exclude=build

This is nice, but in my opinion a bit too implicit. So I removed the particular
test, which is indeed redundant, but kept the other two python files as they
were.

[Sebastian Noack, 2016-05-17 16:04:10]

LGTM

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv.py
File patchconv/patchconv.py (right):

https://codereview.adblockplus.org/29341347/diff/29341348/patchconv/patchconv...
patchconv/patchconv.py:74: def script():
On 2016/05/17 15:57:30, Vasily Kuznetsov wrote:
> On 2016/05/12 23:48:51, Sebastian Noack wrote:
> > On 2016/05/12 23:25:16, Vasily Kuznetsov wrote:
> > > On 2016/05/12 21:44:26, Sebastian Noack wrote:
> > > > This seems to be an unconventional name for an entry point function, or
is
> > it
> > > > indeed common? As far as I can tell people usually call it main()?
> > > > 
> > > > Also I wonder whether it makes quickly testing changes unnecessarily
> > > complicated
> > > > since you cannot run this module as script without installing it first.
> > > 
> > > Quickly testing is still pretty easy after `setup.py develop` but you're
> > > probably right that `main` is a more conventional name. Let's make it
> main().
> > > 
> > > Done.
> > 
> > I'm just curious is this common practice? Most Python scripts like this I've
> > seen so far use if __name__ == '__main__'
> 
> if __name__ == '__main__' allows running the script with `python script.py` or
> `python -m package.script`. However, this script is installed via
> `console_scripts` entry point in setup.py, so it can be run simply by typing
> `patchconv`. With this in mind it seems to me that the other ways of running
it
> are a bit redundant. If we want more flexibility, I could add `if __name__ ==
> '__main__': main()` to the end of this file to enable the other options. Do
you
> think it would be useful?

I don't have a strong opinion here.