aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Sirois" <john.sir...@gmail.com>
Subject Re: Review Request 40219: Replace Twitter checkstyle with pants checkstyle.
Date Thu, 12 Nov 2015 20:13:20 GMT


> On Nov. 12, 2015, 12:05 p.m., Zameer Manji wrote:
> > Although the end result LGTM, I'm not sure if having our own custom pants plugin
is the way to go here. Historically we have been very bad at upgrading pants and maintaining
it, I'm afraid that if we add a custom plugin here we won't be able to upgrade pants in the
future at all.
> 
> John Sirois wrote:
>     As you see fit.  I will say that the APIs used here are minimal and historically
stable.  For example, Medium similarly enables checkstyle as well as another, non-default-installed
plugin, `python-eval`, and they have not had to touch their plugin since initial install at
0.0.45 in August when it was released.  The other bit to know is pants is locking down to
1.0.0 as the year closes to isolate more radical changes.  There will be a long-term 1.0.0
branch the Square will be a major user of and a maintainer of with the primary focus being
stability, secondary bugfixes, but ~no new public activity as the pants community charges
on master towards 2.0.
> 
> Zameer Manji wrote:
>     Don't give up on this approach just yet, I'm curious on what the other reviewers
have to say. I think putting the check in the pants layer is far better than what we have
right now. I'm just not sure if we can maintain this going forward. If pants 1.0.0 will maintain
this API that's pretty good to hear.
> 
> John Sirois wrote:
>     I did use more ceremony here than necessary.  The plugin is just the single register.py
file and the 2 entries in pants.ini.  The BUILD files added and the maven-style directory
tree in `build-support/plugins` are not needed.  The BUILDs are only needed to support applying
 style checks, and adding tests to the custom plugin code - those could be dropped.  The directory
structure could be as simple as `build-support/pants_lugins` with an `__init__.py` and `register.py`
in that dir and no more.
> 
> John Sirois wrote:
>     This review is being discussed on the pantsbuild slack and more discussion is needed,
but one other idea is for pants to ship the python checkstyle as a plugin.  This way folks
who want to turn on checkstlye just add this to pants.ini:
>     ```ini
>     plugins: ['pantsbuild.pants.contrib.py_checkstyle==%(pants_version)s']
>     ```
>     
>     I'll provide an update when the discussion settles.  Pants releases every Friday,
so if the community agrees on the plugin approach this could be ready by tomorrow evening
in `0.0.58`.

Alrighty, the pantsbuild conclusion is pants should ship a plugin for py-checkstyle instead
of embedding it in core pants, but turned off and needing a custom plugin to enable like I
did here.
Please hold off on review, I'll be updating the RB tomorrow to use the new plugin (I'm buildmeister
this week, so I'm pretty confident the new plugin will ship tomorrow ;))


- John


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


On Nov. 12, 2015, 1:54 a.m., John Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40219/
> -----------------------------------------------------------
> 
> (Updated Nov. 12, 2015, 1:54 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Joe Smith, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1532
>     https://issues.apache.org/jira/browse/AURORA-1532
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> The built-in pants python checkstyle plugin is turned on by adding a
> small custom plugin that installs the checks in the compile goal.  Now
> style checks run before compile (and thus before tests) and they benefit
> from fingerprinting; ie: if you test your changes, those tests will run
> style checks and when you go to commit, those checks will not be re-run
> by the commit hook (although files you did not test will still need to
> be checked).
> 
> A few production files were fixes up according to style failures coming
> from no space after comment opening '#', unused variables, and
> mis-aligned hanging closing parens.
> 
>  build-support/hooks/pre-commit                                                     
                        |  3 +--
>  build-support/jenkins/build.sh                                                     
                        |  9 +++++----
>  build-support/{python/checkstyle-check => plugins/3rdparty/python/BUILD}        
                           | 13 ++-----------
>  build-support/{python/checkstyle-check => plugins/src/main/python/apache/__init__.py}
                      | 12 +-----------
>  build-support/{python/checkstyle-check => plugins/src/main/python/apache/aurora/__init__.py}
               | 12 +-----------
>  build-support/{python/checkstyle-check => plugins/src/main/python/apache/aurora/pants/__init__.py}
         | 12 +-----------
>  build-support/{python/checkstyle-check => plugins/src/main/python/apache/aurora/pants/optional/BUILD}
      | 18 +++++++-----------
>  build-support/{python/checkstyle-check => plugins/src/main/python/apache/aurora/pants/optional/__init__.py}
| 12 +-----------
>  build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py     
                        | 36 ++++++++++++++++++++++++++++++++++++
>  build-support/python/checkstyle                                                    
                        | 34 ----------------------------------
>  build-support/python/checkstyle-check                                              
                        |  6 +++---
>  pants.ini                                                                          
                        | 43 +++++++++++++++++++++++++++++++++++++++++++
>  src/main/python/apache/aurora/admin/maintenance.py                                 
                        |  2 +-
>  src/main/python/apache/aurora/client/api/__init__.py                               
                        |  4 ++--
>  src/main/python/apache/aurora/client/cli/client.py                                 
                        |  2 +-
>  src/main/python/apache/aurora/client/cli/cron.py                                   
                        |  2 +-
>  src/main/python/apache/thermos/core/process.py                                     
                        |  6 +++---
>  src/main/python/apache/thermos/monitoring/process_collector_psutil.py              
                        |  1 -
>  src/test/python/apache/aurora/admin/test_admin.py                                  
                        |  6 ------
>  src/test/python/apache/aurora/admin/util.py                                        
                        |  2 +-
>  src/test/python/apache/aurora/client/cli/test_task.py                              
                        |  3 +--
>  21 files changed, 111 insertions(+), 127 deletions(-)
> 
> 
> Diffs
> -----
> 
>   build-support/hooks/pre-commit 619fa9e245be49e4e1f21781c0908cbf744b10ea 
>   build-support/jenkins/build.sh 5cd5242a2e4551e356e5ce5d15a0d27beb7e2d4e 
>   build-support/plugins/3rdparty/python/BUILD PRE-CREATION 
>   build-support/plugins/src/main/python/apache/__init__.py PRE-CREATION 
>   build-support/plugins/src/main/python/apache/aurora/__init__.py PRE-CREATION 
>   build-support/plugins/src/main/python/apache/aurora/pants/__init__.py PRE-CREATION

>   build-support/plugins/src/main/python/apache/aurora/pants/optional/BUILD PRE-CREATION

>   build-support/plugins/src/main/python/apache/aurora/pants/optional/__init__.py PRE-CREATION

>   build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py PRE-CREATION

>   build-support/python/checkstyle 61acc22613acece01580761b25afc7a3edb6b845 
>   build-support/python/checkstyle-check b2bfc5dd71193a8056828e9af05a4c16965f32a1 
>   pants.ini 0bd8ec829ae65e2296a122166dea13f315323c9f 
>   src/main/python/apache/aurora/admin/maintenance.py 6d94c923ae37bf6b827519d3505b100af306296b

>   src/main/python/apache/aurora/client/api/__init__.py 6f07a3073a5d422373238619d459fbd09d8adf3d

>   src/main/python/apache/aurora/client/cli/client.py 297fb588808c1eebc32ac3374265ba986dab3436

>   src/main/python/apache/aurora/client/cli/cron.py 6376fd014f2a4da29442b5c2c7eb36578b503ba3

>   src/main/python/apache/thermos/core/process.py fe95cb3be01b47616596bd78cb9a919b2e8bd978

>   src/main/python/apache/thermos/monitoring/process_collector_psutil.py f1ec5a9050ac60700c4a8afa905bcf12a9bd8a44

>   src/test/python/apache/aurora/admin/test_admin.py 8e204ab43c6bf69867ea7c32b0a7ba7fb29c0766

>   src/test/python/apache/aurora/admin/util.py 3570407b51613d0a7b4fde8a4794d88b98e150b5

>   src/test/python/apache/aurora/client/cli/test_task.py 5432a3d5f7e150b12bd75db0dac7a9018e1c6636

> 
> Diff: https://reviews.apache.org/r/40219/diff/
> 
> 
> Testing
> -------
> 
> Ran the checks alot to get things green, but also ran into the commit hook
> failing the new plugin code itself as a serendipitous check:
> ```
> $ git commit
> Performing Python import order check.
> SUCCESS
> Performing Python checkstyle.
> 
> 
> ** PYTHON CHECKSTYLE FAILED
> *
> * For more information please run: build-support/python/checkstyle-check
> *
> **
> 
> $ build-support/python/checkstyle-check
> ...
> 00:48:50 00:01   [compile]
> 00:48:50 00:01     [compile]
> 00:48:50 00:01     [jvm]
> 00:48:50 00:01       [jvm-compilers]
> 00:48:50 00:01         [zinc-pre]
> 00:48:50 00:01         [zinc-post]
> 00:48:50 00:01     [jvm-dep-check]
> 00:48:50 00:01     [pythonstyle]
>                    Invalidated 1 target.
> F401:ERROR   build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py:015
'PythonRequirement' imported but unused
>      |from pants.backend.python.python_requirement import PythonRequirement
> 
> E225:ERROR   build-support/plugins/src/main/python/apache/aurora/pants/optional/register.py:032
missing whitespace around operator
>      |  context_aware_object_factories={'pants_requirement': pants_requirement_factory}
> 
> 
> FAILURE: Python Style issues found
> 
> 
> 00:48:50 00:01   [complete]
>                FAILURE
> 
> ```
> 
> 
> Thanks,
> 
> John Sirois
> 
>


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