Allow running prep_commands in goals other than test

Review Request #3519 — Created March 1, 2016 and submitted

zundel
pants
zundel/prep-command-goals
2991
9ec83e4...
pants-reviews
benjyw, gmalmquist, kwlzn, nhoward_tw

This change refactors the prep_command() target to allow specifying a goal to run in.

That goal must have a subclass of RunPrepCommandBase registered

Deprecates the RunPrepCommand task in favor of RunTestPrepCommand

Added unit and integration tests
CI running at https://travis-ci.org/pantsbuild/pants/builds/113288333

  • 0
  • 0
  • 14
  • 0
  • 14
Description From Last Updated
NH
  1. I like changes that make similar concepts more similar. Nice! I've got a bunch of minor nitpicks, but it looks pretty good.

  2. I like this wording better than the earlier version, but how about this edit?

    This task will compile dependent JVM code unless its 'classpath_product_only' field
    is set to True. If 'classpath_product_only' is True, it will only require the compile_classpath product and not the runtime_classpath product.

    Actually, classpath_product_only is a kind of confusing name. I know it'd expand the scope a little, but how would you feel about flipping and renaming it to something like include_runtime_classpath

    1. I didn't do this. Its a little odd to try to run code without runtime_classpath if you look at precedent in the repo, that's why I made the name 'compile_classpath_only' But if you feel strongly I will change it.

    2. No I don't feel that strongly about it. I'd appreciate it if the docstring used the right field name though. Them being different is why I ended up thinking about it too much.

  3. s/tasked/tasks/?

  4. nit: period.

  5. nit: period.

  6. Please add the passed goal to the message.

  7. src/python/pants/core_tasks/register.py (Diff revision 1)
     
     

    nit: period

  8. Could you add self.goal to the message. Also, should this be a different kind of error? It feels like this is more of a workspace configuration error.

    Can you write a test exercising it?

    1. This really should never happen if the register_options() is working right. But that is a unorthodox way to use register_options() Its more of an internal consistency check so I changed it to an AssertionError

    2. Thanks.

  9. src/python/pants/core_tasks/run_prep_command.py (Diff revision 1)
     
     
     
     

    I'm a little confused about these docstrings since the tasks are registered in core. I think we should get rid of them.

  10. It looks like this assertion is already covered in the places it is used.

  11. It might make sense to extract a couple existence assertion methods for file existence/non-existence to reduce the visual noise in here.

  12. I think these ought to either be setup and reset in setUp and tearDown, or moved up and out of the class body.

    It's alittle confusing here since they are still evaluated statically.

    1. created setUp and tearDown and added a reset() method

  13. nit: could you name the vars here more descriptively? eg tgt2 -> prep_cmd_binary.

  14. It seems like is_prep is both checking that the target is a prep command, and checking the goal associated with it. Maybe it could have a better name.

  15. 
      
ZU
NH
  1. Ship it! After updating the field name anyway.

  2. 
      
ZU
GM
  1. Ship It!
  2. 
      
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

Thansk Nick and Garrett. Commit a8f0ace

Loading...