----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64970/#review198478 -----------------------------------------------------------
src/python/cli_new/tox.ini Lines 1-22 (patched) <https://reviews.apache.org/r/64970/#comment278637> I would pull the addition of this file out to it's own separate commit right after the commit changing `lib/tox.ini`. src/python/cli_new/tox.ini Lines 22 (patched) <https://reviews.apache.org/r/64970/#comment278620> Let's make `tests` a module and add it in here as well. Also let's explicitly add the `.py` files under `bin` to this list as well. That way all relevant python files will be linted by default when invoking tox directly here instead of via `support/mesos-style.py` src/python/lib/tox.ini Line 1 (original), 1 (patched) <https://reviews.apache.org/r/64970/#comment278636> I would pull the changing of this file out to it's own separate commit at the beginning of the chain of commits for this RR. src/python/lib/tox.ini Lines 22 (patched) <https://reviews.apache.org/r/64970/#comment278621> Let's add `setup.py` to this list by default as well. support/mesos-style.py Line 354 (original), 354-356 (patched) <https://reviews.apache.org/r/64970/#comment278622> s/run/lint/g support/mesos-style.py Line 373 (original), 373 (patched) <https://reviews.apache.org/r/64970/#comment278626> If we decide to add `@staticmethod` here, we should do it on all functions that qualify for this in the entire file. support/mesos-style.py Line 374 (original), 375-378 (patched) <https://reviews.apache.org/r/64970/#comment278623> Thi docstring is incorrect. support/mesos-style.py Lines 379-394 (patched) <https://reviews.apache.org/r/64970/#comment278625> Let's change this to: ``` tox_cmd = [os.path.dirname(__file__), '.virtualenv', 'bin', 'tox')] tox_cmd += ['-c', configfile] if tox_env is not None: tox_cmd += ['-e', tox_env] if recreate: tox_cmd += ['--recreate'] tox_cmd += ['--'] + args ``` I understand that using a generator would make it immutable and doing so is likely preferable in many circumstances, but I think here is reduces readability and is inconsistent with the rest of the code base. support/mesos-style.py Line 380 (original), 400 (patched) <https://reviews.apache.org/r/64970/#comment278634> Let's consider doing a blanket addition of @staticmethod where appropriate in a separate commit so that the code always remains consistent. support/mesos-style.py Line 382 (original), 407 (patched) <https://reviews.apache.org/r/64970/#comment278630> Let's consider renaming this function. Since it doesn't actually take the same parameters as `run_lint()`, I think we can be more descriptive in its naming. support/mesos-style.py Lines 384-385 (original), 409-410 (patched) <https://reviews.apache.org/r/64970/#comment278629> I think that this doc string is incorrect now. It should also indicate what the return value of this function means. support/mesos-style.py Lines 413-414 (patched) <https://reviews.apache.org/r/64970/#comment278631> This could probably be one line. support/mesos-style.py Line 398 (original), 432 (patched) <https://reviews.apache.org/r/64970/#comment278632> This should be in it's own isolated commit before this review request with a good explanation of what it's doing and why it's necessary. support/mesos-style.py Lines 447 (patched) <https://reviews.apache.org/r/64970/#comment278628> Let's consider renaming this function. Since it doesn't actually take the same parameters as `run_lint()`, I think we can be more descriptive in its naming. - Kevin Klues On March 1, 2018, 10:34 p.m., Eric Chung wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64970/ > ----------------------------------------------------------- > > (Updated March 1, 2018, 10:34 p.m.) > > > Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues. > > > Repository: mesos > > > Description > ------- > > Use tox for linting and testing code living uder src/python. > > At the moment, all linting is done through the same `pylint` > installation under support/.virtualenv, which requires ALL dependencies > (i.e. pip-requirements.txt, requirements.in scattered in various > directories) to be installed in the same virtualenv, making things > really messy -- e.g. when I've changed some code under `src/python/lib`, > but don't have the dev virtualenv activated, linting will fail since > none of the dependencies under `src/python/lib` have been installed. > > Using tox, we can solve this problem by distributing a "test spec" > (tox.ini) in each of the python source directories which are aware of > its local dependencies only. To test or lint the code there would be as > simple as running `tox -e py27-lint <file_path>`, and the corresponding > virtualenv and test dependencies would automatically be setup. > > This patch modifies `support/mesos-style.py` to install `tox` in > `support/.virtualenv` and delegates linting to a `tox` call when it sees > python directories that have tox setup for it. Linting for all other > languages will not be effected. > > Testing Done: > 1. intentionally create a lint error, such as extra spaces before a > parens in a python file > 2. run the pre-commit hook and see tox in action > > Reviewed at https://reviews.apache.org/r/64970/ > > > Diffs > ----- > > src/python/cli_new/tox.ini PRE-CREATION > src/python/lib/requirements-test.in > b2b73aab65377d9310797203ea84c5150ae60805 > src/python/lib/tox.ini fd5e89c77c8608fea21e9caad814c6e111ad57db > support/mesos-style.py 47ec36949010fa511d1b3974739c5ad5c03f6f7f > > > Diff: https://reviews.apache.org/r/64970/diff/4/ > > > Testing > ------- > > 1. intentionally create a lint error, such as extra spaces before a parens in > a python file > 2. run the pre-commit hook and see tox in action > > > Thanks, > > Eric Chung > >
