'goal builddict' Target classes "inherit" docstrings, signatures

Review Request #1018 — Created Sept. 11, 2014 and submitted

lahosken
pants
2bff34c...
pants-reviews
patricklaw

Nowadays, our BUILD target implementations use class inheritance + some constructor idioms. But the BUILD Dictionary hasn't kept up. Thus, we have boilerplate docstrings and documented signatures like android_binary(build_type=None, *args, **kwargs)

This change...

Special-cases target handling in src/python/pants/backend/core/tasks/builddictionary.py to go up the inheritence tree, pull apart and re-assemble signatures and docstrings. Also, special-cases leaving out implementation-params we don't want in the user-facing doc (address, build_graph, payload). It doesn't have a general mechanism for leaving stuff out, though. E.g., python_binary's entry thinks it has a sources param (inherited from PythonTarget); mitigated by having a docstring for it saying "overridden: use source instead".

Removes many many redundant docstring fragments. My search wasn't scientific. Probably other removal-worthy pieces lurk.

Adds some docstrings. E.g. python_binary's "sources: overridden". Move some docstrings to superclass for easier inheriting.

https://travis-ci.org/pantsbuild/pants/builds/35031071

  • 0
  • 0
  • 6
  • 0
  • 6
Description From Last Updated
PA
  1. Very nice. lgtm minus the nits

  2. This goes with stdlib imports

  3. This goes between stdlib imports and pants imports

  4. 2 blank lines between global scope definitions

  5. Why the replacement? Is this to guard against your own machinery picking up this example?

    1. Why the replacement?

      No good reason. I was staring at Sphinx's autodoc code for a couple of hours, and got silly?

  6. This is quite difficult to read and I suspect it will be difficult to maintain. At the very least, it should be commented more thoroughly.

    In general, lots of continues and state dependent looping is difficult to reason about. It seems like you want a more sophisticated regular expression, or even a parser.

    1. Changed. Mmmaybe fixed. If I made things worse, I can go back to original-code-but-with-more-comments.

  7. Any reason this isn't Target? AbstractTarget is just a bucket of abstraction leaks, and it needs to go away eventually.

    1. No good reason, only ignorance.

  8. 
      
LA
LA
Review request changed

Status: Closed (submitted)

Loading...