aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@twopensource.com>
Subject Re: Review Request 27852: Ensure run verb returns an exit code.
Date Fri, 14 Nov 2014 00:09:04 GMT


> On Nov. 11, 2014, 2:55 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/cli/task.py, line 72
> > <https://reviews.apache.org/r/27852/diff/1/?file=757426#file757426line72>
> >
> >     Should we just make this the default behavior?  There's at least 31 locations
that do this, seems like a catch-all would be useful.  If we do this, i'd recommend you remove
all `return EXIT_OK` lines, so only proceed if you're okay with that.
> >     
> >     Looks like the relevant changes would be in:
> >     src/main/python/apache/aurora/client/cli/standalone_client.py
> >     src/main/python/apache/aurora/client/cli/client.py
> 
> Zameer Manji wrote:
>     I don't see how this approach will work. This problem of not returning an exit code
comes from the subclasses not implementing the method correctly not that we dispatch to the
super class's implementation. In addition I'm not comfortable mixing in this (minor) change
with a larger structural change.
> 
> Bill Farner wrote:
>     Are we talking about different things?  I'm just suggesting a change like this, around
sys.exit:
>     
>     ```
>        client = AuroraCommandLine()
>        if len(sys.argv) == 1:
>          sys.argv.append("help")
>     -  sys.exit(client.execute(sys.argv[1:]))
>     +  exit_code = client.execute(sys.argv[1:])
>     +  sys.exit(0 if exit_code is None else exit_code)
>      
>      if __name__ == '__main__':
>        proxy_main()
>     ```
>     
>     This is basically just changing the contract to exit 0 if no explicit exit code is
returned.  Given we're working in a language that doesn't have a compiler to enforce a return
value, i think this is the right thing to do.

The python philisohpy is 'explicit over implicit'. I feel that returning None is an error
(that we should catch in our tests) and we should explicitly returning the exit code.


- Zameer


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


On Nov. 10, 2014, 6:50 p.m., Zameer Manji wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27852/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2014, 6:50 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-923
>     https://issues.apache.org/jira/browse/AURORA-923
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Ensure run verb returns an exit code.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/cli/task.py 91175facdc8ccccc9fd59ab66781f86ee8b5940a

>   src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649

>   src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b

> 
> Diff: https://reviews.apache.org/r/27852/diff/
> 
> 
> Testing
> -------
> 
> ./pants build src/test/python/apache/aurora/client/cli::
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message