[l2tdevtools] Add review script functionality to l2tdevtools

[l2tdevtools] Add review script functionality to l2tdevtools

[onager, 2017-10-12 18:51:08]

Code updated.

[Joachim Metz, 2017-10-13 04:29:29]

Largely style nits, to have it match with the remaining l2tdevtools code style.

I assume most of the code is a 1-to-1 copy from l2treviewtools?

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
File l2tdevtools/helpers/__init__.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
l2tdevtools/helpers/__init__.py:1: # -*- coding: utf-8 -*-
maybe rename this folder to cli_helpers (we can do this in a follow up CL).

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
File l2tdevtools/helpers/project.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
l2tdevtools/helpers/project.py:18: 
- white line

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
File l2tdevtools/helpers/pylint.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
l2tdevtools/helpers/pylint.py:2: """Helper for interacting with pylint."""
+ 1 white line after docstring for consistency

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py
File l2tdevtools/lib/errors.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py...
l2tdevtools/lib/errors.py:9: class ConnectionError(Error):
There is a built-in ConnectionError

https://docs.python.org/3/library/exceptions.html

Maybe rename this class name? ConnectivityError ?

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py
File tests/helpers/cli.py (right):

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py#newc...
tests/helpers/cli.py:26: self.assertEqual(stderr, b'')
for consistency add shebang and:

if __name__ == '__main__':
  unittest.main()

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs.py
File tests/helpers/readthedocs.py (right):

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs...
tests/helpers/readthedocs.py:1: # -*- coding: utf-8 -*-
for consistency add shebang

[Joachim Metz, 2017-10-13 04:29:29]

Largely style nits, to have it match with the remaining l2tdevtools code style.

I assume most of the code is a 1-to-1 copy from l2treviewtools?

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
File l2tdevtools/helpers/__init__.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
l2tdevtools/helpers/__init__.py:1: # -*- coding: utf-8 -*-
maybe rename this folder to cli_helpers (we can do this in a follow up CL).

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
File l2tdevtools/helpers/project.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
l2tdevtools/helpers/project.py:18: 
- white line

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
File l2tdevtools/helpers/pylint.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
l2tdevtools/helpers/pylint.py:2: """Helper for interacting with pylint."""
+ 1 white line after docstring for consistency

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py
File l2tdevtools/lib/errors.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py...
l2tdevtools/lib/errors.py:9: class ConnectionError(Error):
There is a built-in ConnectionError

https://docs.python.org/3/library/exceptions.html

Maybe rename this class name? ConnectivityError ?

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py
File tests/helpers/cli.py (right):

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py#newc...
tests/helpers/cli.py:26: self.assertEqual(stderr, b'')
for consistency add shebang and:

if __name__ == '__main__':
  unittest.main()

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs.py
File tests/helpers/readthedocs.py (right):

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs...
tests/helpers/readthedocs.py:1: # -*- coding: utf-8 -*-
for consistency add shebang

[onager, 2017-10-13 21:16:50]

This is mostly a copy from l2tdevtools, but there's now a --project-path arg in
tools/review.py that works out which project is being worked on.

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
File l2tdevtools/helpers/__init__.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
l2tdevtools/helpers/__init__.py:1: # -*- coding: utf-8 -*-
On 2017/10/13 04:29:29, Joachim Metz wrote:
> maybe rename this folder to cli_helpers (we can do this in a follow up CL).

Acknowledged. I suspect there's some deduplication of code we can do, given
there are multiple tools touching github etc.

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
File l2tdevtools/helpers/project.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
l2tdevtools/helpers/project.py:18: 
On 2017/10/13 04:29:29, Joachim Metz wrote:
> - white line

Done.

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
File l2tdevtools/helpers/pylint.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
l2tdevtools/helpers/pylint.py:2: """Helper for interacting with pylint."""
On 2017/10/13 04:29:29, Joachim Metz wrote:
> + 1 white line after docstring for consistency

Done.

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py
File l2tdevtools/lib/errors.py (right):

https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py...
l2tdevtools/lib/errors.py:9: class ConnectionError(Error):
On 2017/10/13 04:29:29, Joachim Metz wrote:
> There is a built-in ConnectionError
> 
> https://docs.python.org/3/library/exceptions.html
> 
> Maybe rename this class name? ConnectivityError ?

Done.

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py
File tests/helpers/cli.py (right):

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py#newc...
tests/helpers/cli.py:26: self.assertEqual(stderr, b'')
On 2017/10/13 04:29:29, Joachim Metz wrote:
> for consistency add shebang and:
> 
> if __name__ == '__main__':
>   unittest.main()

Done.

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs.py
File tests/helpers/readthedocs.py (right):

https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs...
tests/helpers/readthedocs.py:1: # -*- coding: utf-8 -*-
On 2017/10/13 04:29:29, Joachim Metz wrote:
> for consistency add shebang

Done.

[onager, 2017-10-13 21:17:07]

On 2017/10/13 21:16:50, onager wrote:
> This is mostly a copy from l2tdevtools, but there's now a --project-path arg
in
> tools/review.py that works out which project is being worked on.
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
> File l2tdevtools/helpers/__init__.py (right):
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/__ini...
> l2tdevtools/helpers/__init__.py:1: # -*- coding: utf-8 -*-
> On 2017/10/13 04:29:29, Joachim Metz wrote:
> > maybe rename this folder to cli_helpers (we can do this in a follow up CL).
> 
> Acknowledged. I suspect there's some deduplication of code we can do, given
> there are multiple tools touching github etc.
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
> File l2tdevtools/helpers/project.py (right):
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/proje...
> l2tdevtools/helpers/project.py:18: 
> On 2017/10/13 04:29:29, Joachim Metz wrote:
> > - white line
> 
> Done.
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
> File l2tdevtools/helpers/pylint.py (right):
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/helpers/pylin...
> l2tdevtools/helpers/pylint.py:2: """Helper for interacting with pylint."""
> On 2017/10/13 04:29:29, Joachim Metz wrote:
> > + 1 white line after docstring for consistency
> 
> Done.
> 
> https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py
> File l2tdevtools/lib/errors.py (right):
> 
>
https://codereview.appspot.com/332850043/diff/20001/l2tdevtools/lib/errors.py...
> l2tdevtools/lib/errors.py:9: class ConnectionError(Error):
> On 2017/10/13 04:29:29, Joachim Metz wrote:
> > There is a built-in ConnectionError
> > 
> > https://docs.python.org/3/library/exceptions.html
> > 
> > Maybe rename this class name? ConnectivityError ?
> 
> Done.
> 
> https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py
> File tests/helpers/cli.py (right):
> 
>
https://codereview.appspot.com/332850043/diff/20001/tests/helpers/cli.py#newc...
> tests/helpers/cli.py:26: self.assertEqual(stderr, b'')
> On 2017/10/13 04:29:29, Joachim Metz wrote:
> > for consistency add shebang and:
> > 
> > if __name__ == '__main__':
> >   unittest.main()
> 
> Done.
> 
>
https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs.py
> File tests/helpers/readthedocs.py (right):
> 
>
https://codereview.appspot.com/332850043/diff/20001/tests/helpers/readthedocs...
> tests/helpers/readthedocs.py:1: # -*- coding: utf-8 -*-
> On 2017/10/13 04:29:29, Joachim Metz wrote:
> > for consistency add shebang
> 
> Done.

Er, a copy from l2treviewtools to l2tdevtools

[onager, 2017-10-13 21:23:21]

Code updated.

[Joachim Metz, 2017-10-14 05:12:04]

LGTM

[onager, 2017-10-19 22:08:19]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: python ./utils/review.py close review