> On March 9, 2015, 8:21 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/base.py, lines 220-221
> > <https://reviews.apache.org/r/31869/diff/1/?file=889598#file889598line220>
> >
> >     Override isn't needed here. You can just leave the class body blank.

Doesn't this help to document the fields that can be expected to be present in 
instances of this class?  IIUC otherwise the code catching the exception has to 
intimately know the code throwing.


> On March 9, 2015, 8:21 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, lines 46-50
> > <https://reviews.apache.org/r/31869/diff/1/?file=889597#file889597line46>
> >
> >     Why this change (removing the root Error class)?

Answering with a question - why did it exist?  It was unused.  Theory of the 
convention aside, i don't see value in keeping something that's never exercised.


> On March 9, 2015, 8:21 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/jobs.py, line 679
> > <https://reviews.apache.org/r/31869/diff/1/?file=889600#file889600line679>
> >
> >     Rather than pass this have it use the instance field or make it a 
> > classmethod.

Went with classmethod.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31869/#review75762
-----------------------------------------------------------


On March 9, 2015, 8:10 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31869/
> -----------------------------------------------------------
> 
> (Updated March 9, 2015, 8:10 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
> 
> 
> Bugs: AURORA-1176
>     https://issues.apache.org/jira/browse/AURORA-1176
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> As indicated by some changes in tests - there were legitimate issues hiding 
> behind this catch.  After this change, the client will allow unhandled 
> exceptions to surface in their full glory.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 194629f61192c1d7d5e7064e9226adf26d03e890 
>   src/main/python/apache/aurora/client/base.py 
> d550c8eeed91f0967e281957b71fcefb0b4cf3b8 
>   src/main/python/apache/aurora/client/cli/__init__.py 
> 4d9ef09749e3075b9d9e2ae1db311e60f7bdb4ee 
>   src/main/python/apache/aurora/client/cli/jobs.py 
> 286b2182d5fe25703882f0b367739ad03d6c8fe8 
>   src/test/python/apache/aurora/client/cli/test_api_from_cli.py 
> b855c3c2d74125738d2106e18a9e9b0ebed6ac4b 
>   src/test/python/apache/aurora/client/cli/test_create.py 
> 459d157155f74b6a3d140bcccc85d3b7f0364367 
>   src/test/python/apache/aurora/client/cli/test_kill.py 
> 7aad34a2fe5591937c5bca890751073439e3a1a6 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> 1806769426a196793481f948892f5474df8dd665 
>   src/test/python/apache/aurora/client/cli/util.py 
> b65970a2717a1f36767c61e5e09c980b04895f01 
> 
> Diff: https://reviews.apache.org/r/31869/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to