> On Oct. 16, 2014, 10 a.m., Bill Farner wrote: > > src/main/python/apache/aurora/client/cli/__init__.py, line 114 > > <https://reviews.apache.org/r/26802/diff/1/?file=722890#file722890line114> > > > > Can you try to remove noqa, and if it is suppressing an error, leave a > > comment indicating why the pyflakes errors are suppressed?
Yep, these should've been classmethods. > On Oct. 16, 2014, 10 a.m., Bill Farner wrote: > > src/main/python/apache/aurora/client/cli/__init__.py, line 118 > > <https://reviews.apache.org/r/26802/diff/1/?file=722890#file722890line118> > > > > Ditto > > > > > > also, more concise: > > > > return Context.ERROR_LOG_DIR or '.' Done. > On Oct. 16, 2014, 10 a.m., Bill Farner wrote: > > src/test/python/apache/aurora/client/cli/BUILD, line 140 > > <https://reviews.apache.org/r/26802/diff/1/?file=722891#file722891line140> > > > > remove extra newline Done. > On Oct. 16, 2014, 10 a.m., Bill Farner wrote: > > src/test/python/apache/aurora/client/cli/test_aurora_command_line.py, line > > 34 > > <https://reviews.apache.org/r/26802/diff/1/?file=722892#file722892line34> > > > > I'm not sure what the right answer is here, and we may be too far down > > the monkeypatch road to change tacks in this effort, but overriding > > builtins feels like a special kind of abstraction violation. > > > > Do any other reviewers know the lay of the land well enough to suggest > > a better way? Ideally we would be injecting this functionallity rather > > than patching it, but i don't want to turn a small fix into a giant yak > > shave. > > Maxim Khutornenko wrote: > Seems like the appropriate use of patching given the current > implementation. Refactoring here would only hurt readability and would rather > feel unnatural. > > Joshua Cohen wrote: > +1, I'm not an expert on what is or isn't pythonic, but this certainly > seems in line with how I've done testing in other dynamic languages. I'm not > sure cluttering up the interface with explicit injection points for > dependencies is warranted/necessary when you can just patch them in tests? Unless we switch to using interfaces and injection, patching is indeed a ~recommended approach. - Joe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26802/#review56929 ----------------------------------------------------------- On Oct. 15, 2014, 11:02 p.m., Joe Smith wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26802/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2014, 11:02 p.m.) > > > Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji. > > > Bugs: AURORA-827 > https://issues.apache.org/jira/browse/AURORA-827 > > > Repository: aurora > > > Description > ------- > > Set a default for the error log dir > > This has been a weird issue to wrap my head around, it's really using lots > of low-level systems (writing tracebacks to files) so I feel like I've done > some weird things in the tests. Feedback and critque welcome + appreciated. > > > Diffs > ----- > > src/main/python/apache/aurora/client/cli/__init__.py > da9d5b6ba4d22ba1f444341b97bbcfaf7889a4a8 > src/test/python/apache/aurora/client/cli/BUILD > d33e86643a59879c115876c98bd1dc19aa7ae61c > src/test/python/apache/aurora/client/cli/test_aurora_command_line.py > PRE-CREATION > > Diff: https://reviews.apache.org/r/26802/diff/ > > > Testing > ------- > > [tw-172-25-132-201 aurora (yasumoto/error_log_dir_default)]$ ./pants > src/test/python/apache/aurora/client/cli:aurora_command_line > Build operating on top level addresses: > set([BuildFileAddress(/Users/jsmith/workspace/aurora/src/test/python/apache/aurora/client/cli/BUILD, > aurora_command_line)]) > ==================================================================== test > session starts > ===================================================================== > platform darwin -- Python 2.7.5 -- py-1.4.25 -- pytest-2.6.3 > plugins: cov, timeout > collected 1 items > > src/test/python/apache/aurora/client/cli/test_aurora_command_line.py . > > ================================================================== 1 passed > in 0.63 seconds > ================================================================== > src.test.python.apache.aurora.client.cli.aurora_command_line > ..... SUCCESS > > > Thanks, > > Joe Smith > >
