> 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`.
> 
> John Sirois wrote:
>     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 ;))

I'll actually discard this review and send up a new one when the py-checkstyle 
plugin is released to avoid confusion.


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

Reply via email to