Add a preconditions package that supports writing precondtions checks and offers a handful of checks for common needs.

Review Request #118 — Created March 19, 2014 and discarded

jsirois
commons
jsirois:commons/preconditions
253
pants-reviews
benjyw, patricklaw, travis, wickman
So...

This is not pythonic afaict.
It would be implement differently in python 3 with type annotations (though not much differently).

However, I like preconditions checks and we have a on of these in pants that are bolier-platey and varying quality and correctness.
This sort of thing would allow those bad parts to go away.

Happy to move this to pants if its not appropriate for commons.
Happy to ditch it altogether if pythonistas say blasphemy.

It entertained me in airports over the last week and a bit this morning and just now.

commit 56662ff4d789247bf0ac6764df0f5628dcb1e691
Author: John Sirois <jsirois@twitter.com>
Date:   Tue Mar 18 19:54:53 2014 -0600

    Add a preconditions package that supports writing precondtions
    checks and offers a handful of checks for common needs.
    
    TODO: possibly add fine-grained whitelist and/or blacklist
    support akin to logging configurtions that allow just turning
    on checks for a critical set of user-facing functions or else
    targeting a performance critical set of function for disabling
    of checks.

 3rdparty/python/BUILD                                     |   1 +
 src/python/twitter/common/preconditions/BUILD             |  39 ++++++
 src/python/twitter/common/preconditions/checker.py        | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/python/twitter/common/preconditions/checks.py         | 219 +++++++++++++++++++++++++++++++
 tests/python/twitter/common/preconditions/BUILD           |  25 ++++
 tests/python/twitter/common/preconditions/test_checker.py | 284 ++++++++++++++++++++++++++++++++++++++++
 tests/python/twitter/common/preconditions/test_checks.py  | 261 +++++++++++++++++++++++++++++++++++++
 7 files changed, 1181 insertions(+)
$ PANTS_PY_COVERAGE=1 ./pants tests/python/twitter/common/preconditions/ -v
Build operating on targets: OrderedSet([PythonTests(tests/python/twitter/common/preconditions/BUILD:preconditions)])
============================================================================================================================== test session starts ===============================================================================================================================
platform linux2 -- Python 2.7.5 -- py-1.4.20 -- pytest-2.5.2 -- /home/jsirois/dev/3rdparty/jsirois-commons/build-support/pants.venv/bin/python2.7
collected 21 items 

tests/python/twitter/common/preconditions/test_checker.py:80: CheckTest.test_disable PASSED
tests/python/twitter/common/preconditions/test_checker.py:56: CheckTest.test_improper_usage PASSED
tests/python/twitter/common/preconditions/test_checker.py:125: CheckTest.test_one_of_improper_usage PASSED
tests/python/twitter/common/preconditions/test_checker.py:98: CheckTest.test_one_of_usage PASSED
tests/python/twitter/common/preconditions/test_checker.py:36: CheckTest.test_usage PASSED
tests/python/twitter/common/preconditions/test_checker.py:179: ArgCheckTest.test_improper_usage PASSED
tests/python/twitter/common/preconditions/test_checker.py:234: ArgCheckTest.test_kwargs PASSED
tests/python/twitter/common/preconditions/test_checker.py:194: ArgCheckTest.test_partial PASSED
tests/python/twitter/common/preconditions/test_checker.py:249: ArgCheckTest.test_positional_varargs_kwargs PASSED
tests/python/twitter/common/preconditions/test_checker.py:145: ArgCheckTest.test_usage PASSED
tests/python/twitter/common/preconditions/test_checker.py:219: ArgCheckTest.test_varargs PASSED
tests/python/twitter/common/preconditions/test_checks.py:79: ChecksTest.test_bool PASSED
tests/python/twitter/common/preconditions/test_checks.py:60: ChecksTest.test_check_type PASSED
tests/python/twitter/common/preconditions/test_checks.py:249: ChecksTest.test_directory PASSED
tests/python/twitter/common/preconditions/test_checks.py:236: ChecksTest.test_file_path PASSED
tests/python/twitter/common/preconditions/test_checks.py:119: ChecksTest.test_iterable PASSED
tests/python/twitter/common/preconditions/test_checks.py:145: ChecksTest.test_mapping PASSED
tests/python/twitter/common/preconditions/test_checks.py:177: ChecksTest.test_maybe_list PASSED
tests/python/twitter/common/preconditions/test_checks.py:49: ChecksTest.test_not_none PASSED
tests/python/twitter/common/preconditions/test_checks.py:215: ChecksTest.test_path PASSED
tests/python/twitter/common/preconditions/test_checks.py:90: ChecksTest.test_string PASSED
---------------------------------------------------------------------------------------------------------------- coverage: platform linux2, python 2.7.5-final-0 -----------------------------------------------------------------------------------------------------------------
Name                                                   Stmts   Miss Branch BrMiss  Cover
----------------------------------------------------------------------------------------
/tmp/tmp5yL5qb/twitter/common/preconditions/__init__       1      0      0      0   100%
/tmp/tmp5yL5qb/twitter/common/preconditions/checker      159      3     64      1    98%
/tmp/tmp5yL5qb/twitter/common/preconditions/checks        83      2     30      0    98%
----------------------------------------------------------------------------------------
TOTAL                                                    243      5     94      1    98%
Coverage HTML written to dir /home/jsirois/dev/3rdparty/jsirois-commons/dist/coverage/tests/python/twitter/common/preconditions/preconditions

=========================================================================================================================== 21 passed in 0.43 seconds ============================================================================================================================
tests.python.twitter.common.preconditions.preconditions                         .....   SUCCESS
  • 1
  • 0
  • 0
  • 0
  • 1
Description From Last Updated
This won't work with generators or anything that's more stream-like, even though they'll pass the type-checks >>> def a(): ... KE kevints
KE
  1. Drive-by review! In general I'm a big fan of these type of preconditions and would favor having such a library included in commons for consumption from Aurora's client and executor to replace the many adhoc places we make these checks now. On the Aurora scheduler (Java) side they've tended to be good documentation of what exactly is expected and are almost always worth the extra keystrokes. And I think they're Pythonic because "explicit is better than implicit", but I've been writing enough Java recently that my mind may have addled here.
  2. This won't work with generators or anything that's more stream-like, even though they'll pass the type-checks
    
    >>> def a():
    ...   yield 1
    ...   yield 2
    ... 
    >>> b = a()
    >>> next(iter(b))
    1
    >>> next(iter(b))
    2
    >>> import collections
    >>> isinstance(b, collections.Iterable)
    True
    
    Looking through the collections docs it doesn't seem like there's a single ABC that captures "rewindability", but checking for (collections.Sequence, collections.Mapping, collections.Set) seems to be closer to what you might want. Seems potentially thorny enough to document thought.
  3. 
      
BE
  1. I'll defer judgement on whether we'd want to use this in Pants to Patrick, since he's the greater Python expert. However I am leery of trying to make Python into something it's not. If we wanted type checks we should have written Pants in Java. We need to embrace, not avoid, the dynamic nature of Python, and save type checks for the few places where they might really be needed.
  2. 
      
WI
  1. It's a little unfair to call this a type-checking library, though that is one of the possible use-cases of it.  This is more of a DRY library, and could help reduce the proliferation of boilerplate.  That being said, I'm also not convinced it should be in twitter.common initially.  As I usually tell other engineers trying to put stuff in twitter.common -- put it inside your project or in common_internal (a Twitter-ism) and once it reaches ubiquity or finds use in another project, then it can be graduated to twitter.common.  We should place a high bar on twitter.common.  There are a few things there that probably shouldn't be or at the very least not be a top-level project (things that come to mind: java, resourcepool, git, jira, confluence, rwbuf, util...)
    1. Sounds good to me - I'll discard this review and only resurrect a pans branch if pants folks decide they like the idea.
  2. 
      
JS
Review request changed

Status: Discarded

Loading...