Instead of defaulting task-related knobs for JavaThriftLibrary, pull the defaulting into a Task helper.

Review Request #244 — Created April 17, 2014 and submitted

jsirois
pants
jsirois/thrift/flex_defaults
66
pants-reviews
areitz, benjyw, patricklaw, travis
commit 3201b1a11a5f3a5a871e5ac9d58a362375fb3868
Author: John Sirois <jsirois@twitter.com>
Date:   Thu Apr 17 13:22:29 2014 -0700

    Instead of defaulting task-related knobs for JavaThriftLibrary, pull the defaulting into a Task helper.
    
    This solves half the problem of the JavaThriftLibrary target knowing about the Tasks that use it.
    In particular it solves acute pain on the default value side and allows a whole repo to configure
    a non-standard default compiler, target language or target rpc_style.
    
    Also re-name ThriftGen to the more accurate ApacheThriftGen.
    Finally - restore ScroogeGen's use of the _validate helper - this
    was lost in the final Twitter -> OSS merge.

 src/python/pants/commands/BUILD                                |  2 +-
 src/python/pants/commands/goal.py                              |  4 +--
 src/python/pants/targets/BUILD                                 |  1 +
 src/python/pants/targets/java_thrift_library.py                | 59 +++++++++++++++++++++------------
 src/python/pants/tasks/BUILD                                   |  4 +--
 src/python/pants/tasks/{thrift_gen.py => apache_thrift_gen.py} |  7 ++--
 src/python/pants/tasks/scrooge_gen.py                          | 47 ++++++++++++++++++--------
 tests/python/pants_test/targets/BUILD                          | 11 ++++++
 tests/python/pants_test/targets/test_java_thrift_library.py    | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/python/pants_test/tasks/BUILD                            |  1 +
 tests/python/pants_test/tasks/test_scrooge_gen.py              |  9 +++--
 11 files changed, 189 insertions(+), 46 deletions(-)
New unit:
$ ./pants tests/python/pants_test/targets:java_thrift_library -v
Build operating on targets: OrderedSet([PythonTests(tests/python/pants_test/targets/BUILD:java_thrift_library)])
============================================================================================================================== test session starts ===============================================================================================================================
platform linux3 -- Python 2.6.8 -- py-1.4.20 -- pytest-2.5.2 -- /home/jsirois/dev/3rdparty/pyenv/versions/2.6.8/bin/python
plugins: cov
collected 4 items 

tests/python/pants_test/targets/test_java_thrift_library.py:74: JavaThriftLibraryDefaultsTest.test_configured_defaults PASSED
tests/python/pants_test/targets/test_java_thrift_library.py:86: JavaThriftLibraryDefaultsTest.test_explicit_values PASSED
tests/python/pants_test/targets/test_java_thrift_library.py:68: JavaThriftLibraryDefaultsTest.test_hardwired_defaults PASSED
tests/python/pants_test/targets/test_java_thrift_library.py:63: JavaThriftLibraryDefaultsTest.test_invalid PASSED

============================================================================================================================ 4 passed in 0.24 seconds ============================================================================================================================


And ci:
$ ./build-support/bin/ci.sh
JS
TR
  1. 
      
  2. 
      
JS
  1. Thanks - merged
  2. 
      
BE
  1. Looks good! Nothing to fix in this change, but see questions/comments inline. 
  2. To be dealt with in a future change, but shouldn't it be JvmThriftLibrary? Or, really, just ThriftLibrary? We can infer the type of the synthetic target from what the compiler outputs, but that's a property of the compiler+its settings, not of the thrift source target.
    1. Absolutely: https://github.com/pantsbuild/pants/issues/67
  3. Love it!
  4. Yep, and we have our own custom thrift->scala codegen.
    
    Now that I think of it, that codegen (Spindle) is open-sourced now, so we could probably add it as a proper sibling to scrooge and save on some of our custom plugin stuff.
  5. Love it!!
  6. In general do we prefer """ over '''? pep8 is silent on the issue, but pep257 requires """ for docstrings, so maybe it makes sense to standardize on them?
    1. We definitely standardize on """""" docstrings, but I know I attempt to use ' or ''' eveywhere else by default - but for no good reason.  I'm ambivalent to what the standard is but agree it behooves us to have them and start writing them down in the doc tree to be pointed at by the contributing docs.
  7. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...