add public api markers

Review Request #3417 — Created Feb. 3, 2016 and submitted

molsen
pants
pants-reviews
benjyw, kwlzn, patricklaw, stuhood, zundel

Based on a discussion on pants slack, a method of marking methods as public using docstrings was proposed. This RB is an
example of how this markup would happen. This is part of the effort discussed in:
https://docs.google.com/document/d/1HDIalsQRv2Ds_uylPznX_VPmMdJib-1PdaGNVvF7fu4/edit#

  • Add markers to public api methods and classes

passes on personal fork.
CI green: https://travis-ci.org/pantsbuild/pants/builds/106836984

ST
  1. 
      
  2. When a class is ':API: public', should we just assume that all of its non-_private methods are public as well?

    Would need to change existing classes a bit in order to not expose implementation details of the class... but that should be a much smaller problem, since it only involves changing methods of public classes, rather than renaming private classes.

    1. I'm assuming that we have cases where we have a public class that have some methods that are inproperly named. I'd rather be explicit to remove ambiguity. Then assume that anything not marked public is private.

  3. 
      
MO
BE
  1. So consensus is that we do need to mark individual methods within classes already marked as API?

    1. The consensus I think is to mark the api's using docstrings. It sounds like Stu may feel marking the class as public and not individual methods is sufficient. I'd prefer to be more explicit and mark each method but would love to hear what other people think.

    2. IMO, only marking classes, and then using the usual _ private conventions within a class is easiest. Otherwise you have annoying edgecases to deal with like "a method is public but it's class or constructor aren't", etc. And sealing up classes before marking them public isn't too much of a burden.

    3. Would you consider inherited methods to fall under public if the class is public as well or just methods defined inside that specific class?

    4. My understanding of python pub/priv conventions isn't great. But I think that _ private methods are intended to be accessible from subclasses (ie, protected in JVM terms). If that's the case, then anything public on superclasses would also be public on the subclass.

    5. This sounds reasonable to me. I'll give some time for other people to comment. If no one objects, then I'll update this RB and start marking up the rest of test.

    6. Actually I think _private should mean private - i.e., only used in this class. If a subclass accesses it then it might as well be public, because those subclasses might live in external repos. So for the leading underscore to be at all useful, it has to mean private, not protected. Technically in python a double-underscore prefix "enforces" true privacy through name-mangling, but we've never used that convention, and it would be pretty disruptive to start now.

    7. Why would using a double underscore be disruptive? Are you concerned mostly with breaking existing plugins?

    8. Having thought about this some more, I'm in favor of marking individual API methods as such, rather than relying on them not beginning with an underscore, as long as it's not too much trouble. Because there is an important distinction between "public within pants" (other classes within the repo can use this) and "public API". The former can be modified without a major version increase, and the latter cannot. So it would be great not to have to treat members as API just because some pants-repo-only class needs to access them.

    9. I don't think it will be to much trouble to add this to methods as well. Since its a docstring its a pretty lightweight and low risk change.

  2. 
      
BE
  1. Ship It!
  2. 
      
PA
  1. Ship It!
  2. 
      
ST
  1. Ship It!
    1. I think that marking on a method-by-method basis is going to become tedious, but this is a step in the right direction, so let's do it. Easy to change later if need be.

  2. 
      
MO
Review request changed

Status: Closed (submitted)

Change Summary:

commit 0fc915e591ebe7771a9df7e42c7684e210f481b9

Loading...