mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 64970: Use tox for linting and testing code living uder src/python.
Date Thu, 01 Mar 2018 23:20:52 GMT

-----------------------------------------------------------
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
> 
>


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