> On Aug. 8, 2014, 2:05 a.m., Brian Wickman wrote: > > the following are high level comments, mostly around style and > > maintainability which are important for community projects with large > > numbers of committers. not all recommendations need to be taken now, but > > doing so sooner than later will save time in the future. > > > > 1) I recommend integrating flake8 and isort into your test suite (either > > via nose or tox) -- this will catch lots of bugs up front and reduce the > > burden from reviewers of keeping style consistent. > > > > 2) At a minimum, add 'from __future__ import absolute_import, > > print_function' to each file. This will promote polyglot 2.x + 3.x by > > throwing SyntaxErrors for statement prints and prevent ambiguous imports. > > > > 3) There are a lot of places where you use itertools.i* and map/filter etc. > > This style of code, while written with the best of intentions re: > > performance, may be premature optimization and introduces subtle bugs that > > make it hard to transition to 3.x. I've tried to point out a bunch of > > these cases but will have certainly missed some. > > > > 4) I recommend against using gevent for mesos.cli -- afaict it is the only > > requirement here that is not pure python, and it's only used in a couple > > places where subprocess + ThreadPool would be sufficient. Only > > recommending this from my experience maintaining egg/wheel dependencies > > across a large engineering organization. C extensions = pain for tools > > intended to run on dev laptops, less so for applications in a homogenous > > production environment. > >
My goodness. Thanks for taking the time, all great feedback! I'll get it updated =) - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24469/#review49996 ----------------------------------------------------------- On Aug. 8, 2014, 12:37 a.m., Thomas Rampelberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24469/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2014, 12:37 a.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: mesos-1016 > https://issues.apache.org/jira/browse/mesos-1016 > > > Repository: mesos-git > > > Description > ------- > > This is a re-implementation of the CLI tools that removes the dependencies on > compiled code and implements everything purely in python. You will now be > able to `pip install mesos.cli` and get these tools anywhere (such as > developer's laptops who don't have mesos itself installed or even a windows > machine). > > The interface has changed and the tool has been made task centric. You can > configure the master you'd like to use and then ignore which framework a task > is running under completely. > > > Diffs > ----- > > src/Makefile.am 39af0365e429b8d08addadb09ee18080a19625f8 > src/cli/mesos-cat 73dc63ebc2fab9150f4dd691e10defaf989b9e6b > src/cli/mesos-ps ddd9ec5dd0045d168ee4ed840194fe18c304b56a > src/cli/mesos-scp 77b8557d8ca33960d9135ad4fa6bfe3dcd087108 > src/cli/mesos-tail 256a804b98b2efb2fb0256635449b36a3a4d0a6b > src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86 > src/cli/python/mesos/__init__.py 028b0d27fb193bac96f2a6a3201ee4cc8fd369ef > src/cli/python/mesos/cli.py 857059e2c12ed7f1419dfbf0d11dda0ff9fae235 > src/cli/python/mesos/futures.py da2f4ceb72f4d8f1a1a48b0c31111a1723b2f638 > src/cli/python/mesos/http.py 0e19aa8dd6595a9b292189364fd51fb9b3bfb285 > src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b > src/python/cli/README.rst PRE-CREATION > src/python/cli/bin/mesos-zsh-completion.sh PRE-CREATION > src/python/cli/docs/debugging.md PRE-CREATION > src/python/cli/mesos/__init__.py PRE-CREATION > src/python/cli/mesos/cli/__init__.py PRE-CREATION > src/python/cli/mesos/cli/cfg.py PRE-CREATION > src/python/cli/mesos/cli/cli.py PRE-CREATION > src/python/cli/mesos/cli/cmds/cat.py PRE-CREATION > src/python/cli/mesos/cli/cmds/completion.py PRE-CREATION > src/python/cli/mesos/cli/cmds/config.py PRE-CREATION > src/python/cli/mesos/cli/cmds/events.py PRE-CREATION > src/python/cli/mesos/cli/cmds/find.py PRE-CREATION > src/python/cli/mesos/cli/cmds/head.py PRE-CREATION > src/python/cli/mesos/cli/cmds/help.py PRE-CREATION > src/python/cli/mesos/cli/cmds/ls.py PRE-CREATION > src/python/cli/mesos/cli/cmds/ps.py PRE-CREATION > src/python/cli/mesos/cli/cmds/resolve.py PRE-CREATION > src/python/cli/mesos/cli/cmds/scp.py PRE-CREATION > src/python/cli/mesos/cli/cmds/ssh.py PRE-CREATION > src/python/cli/mesos/cli/cmds/state.py PRE-CREATION > src/python/cli/mesos/cli/cmds/tail.py PRE-CREATION > src/python/cli/mesos/cli/exceptions.py PRE-CREATION > src/python/cli/mesos/cli/log.py PRE-CREATION > src/python/cli/mesos/cli/main.py PRE-CREATION > src/python/cli/mesos/cli/master.py PRE-CREATION > src/python/cli/mesos/cli/mesos_file.py PRE-CREATION > src/python/cli/mesos/cli/slave.py PRE-CREATION > src/python/cli/mesos/cli/ssh.py PRE-CREATION > src/python/cli/mesos/cli/state.py PRE-CREATION > src/python/cli/mesos/cli/task.py PRE-CREATION > src/python/cli/mesos/cli/util.py PRE-CREATION > src/python/cli/mesos/cli/zookeeper.py PRE-CREATION > src/python/cli/setup.cfg PRE-CREATION > src/python/cli/setup.py PRE-CREATION > src/python/cli/tests/__init__.py PRE-CREATION > src/python/cli/tests/data/browse.json PRE-CREATION > src/python/cli/tests/data/config.json PRE-CREATION > src/python/cli/tests/data/master-host PRE-CREATION > src/python/cli/tests/data/master.pb PRE-CREATION > src/python/cli/tests/data/master_state.json PRE-CREATION > src/python/cli/tests/data/sandbox/stderr PRE-CREATION > src/python/cli/tests/data/sandbox/stdout PRE-CREATION > src/python/cli/tests/data/slave-20140619-151434-16842879-5050-1196-0.json > PRE-CREATION > src/python/cli/tests/data/slave_statistics.json PRE-CREATION > src/python/cli/tests/integration/__init__.py PRE-CREATION > src/python/cli/tests/integration/test_cat.py PRE-CREATION > src/python/cli/tests/integration/test_completion.py PRE-CREATION > src/python/cli/tests/integration/test_config.py PRE-CREATION > src/python/cli/tests/integration/test_find.py PRE-CREATION > src/python/cli/tests/integration/test_head.py PRE-CREATION > src/python/cli/tests/integration/test_ls.py PRE-CREATION > src/python/cli/tests/integration/test_ps.py PRE-CREATION > src/python/cli/tests/integration/test_resolve.py PRE-CREATION > src/python/cli/tests/integration/test_scp.py PRE-CREATION > src/python/cli/tests/integration/test_ssh.py PRE-CREATION > src/python/cli/tests/integration/test_state.py PRE-CREATION > src/python/cli/tests/integration/test_tail.py PRE-CREATION > src/python/cli/tests/utils.py PRE-CREATION > src/python/cli/tox.ini PRE-CREATION > > Diff: https://reviews.apache.org/r/24469/diff/ > > > Testing > ------- > > > Thanks, > > Thomas Rampelberg > >
