De-publicize a FAPP private method.

Review Request #3750 — Created April 22, 2016 and submitted

jsirois
pants
jsirois/distribution/fix_tests/clarify_api
2894, 3260
pants-reviews
molsen, nhoward_tw, zundel
This also fixes some >100 cols formatting.

 src/python/pants/java/distribution/distribution.py | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

Locally has the same set of failures before and after:
./pants test tests/python/pants_test/java/distribution

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/125125707

JS
  1. I'm not sure how the `:API: public` protocol is playing out in practice.  I think publicizing this method was just an honest mistake in https://rbcommons.com/s/twitter/r/3655/.
    1. This looks safe to move even under the new 1.0 release protocols. It isn't marked API:public so we aren't breaking our guarantee.

    2. Well, none of its methods are and folks rely on them fwict.  I was assuming a class marked public had all its "public" methods assumed also to be `:API: public`.
    3. ... and nested types too, folks need to be able rely on the nested Error type in except clauses.
    4. I tried to be pretty conservative with our public api's, its easier to make something public later than change our mind. As far as methods inside a class the methods need to be marked public as well if we expect them to be used. It would be nice if we explicitly named methods that they intend to be private with the preceding underscore but a lot of our code doesn't seem to follow that convention.

    5. What does it mean then for `DistributionLocator` to be an `:API: public` type with none of its methods marked `:API: public`?  Its un-useable that way.
    6. I don't remember the specifics of that class but in some cases people use it only to do isinstance checks or to intantiate an object and pass it off to something else.

    7. Yea, it seemed useful to differentiate between having class references and actually using methods. So much in python is based on what in Java would be reflection on types.

      It looks like both DistributionLocator and Distribution need a few methods exposed though.

    8. I don't understand the 1st sentence.  The vast majority of use of types and types alone is in except clauses.  In almost every other case, if a type is used in a type test, that's done to access a method on the type or else raise; ie: runtime type-check.  I think its safe as a general rule when creating or reviewing `:API: public` changes, that marking a non-exception type public but none of its methods is almost always wrong.  We simply don't use sentinel types except for exception types.
    9. When types are used the way that labels "labels" used to be used, you get examples in tasks where types are used only as filters, after which only methods/fields of the Target base class are accessed:
      src/python/pants//backend/python/thrift_builder.py
      src/python/pants//backend/project_info/tasks/export.py
      src/python/pants//backend/jvm/targets/import_jars_mixin.py
      src/python/pants//backend/codegen/tasks/jaxb_gen.py

      etc.

      It clearly doesn't make sense for DistributionLocator/et-al though.

    10. Ah yes, thank you.  I totally forgot about the use of sentinel Target types in the codegen 1 source -> many products cases in particular.
  2. 
      
ZU
  1. Ship It!
  2. 
      
MO
  1. Ship It!
  2. 
      
ST
  1. Ship It!
  2. 
      
JS
JS
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit b2a4960730dcf4ba256e679af9f782dcc914832d
Author: John Sirois <john.sirois@gmail.com>
Date:   Sun Apr 24 10:09:33 2016 -0600

    De-publicize a FAPP private method.
    
    This also fixes some >100 cols formatting.
    
    Testing Done:
    Locally has the same set of failures before and after:
    `./pants test tests/python/pants_test/java/distribution`
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/125125707
    
    Bugs closed: 2894, 3260
    
    Reviewed at https://rbcommons.com/s/twitter/r/3750/
Loading...