Add path-to option to depmap. This causes the depmap to only show deps that are ancestors of some specific dep. It reduces depmap output to something more human-readable.

Review Request #1545 — Created Dec. 26, 2014 and submitted

dturner-tw
pants
654467f...
pants-reviews
fkorotkov, jsirois, patricklaw, tejal

Add path-to option to depmap. This causes the depmap to only show deps that are ancestors of some specific dep. It reduces depmap output to something more human-readable.

Ran tests (including new test).

  • 0
  • 0
  • 2
  • 2
  • 4
Description From Last Updated
PA
  1. lgtm sans the issue raised

  2. Dependencies is deprecated in favor of just using target, which is an alias for Target. Hence this isinstance should be vacuously true 100% of the time, except when someone is using a deprecated alias. This sort of raises the question of what the intent of this condition is--I assume to walk through aliases? This behavior is probably broken right now if so--that is, we likely walk through everything regardless, including e.g. python targets. But imo that's the correct behavior, so we should probably simplify this whole task to only care about jvm vs. python dependencies in the situations where it's choosing whether to print third party dep specs. Regardless, this behavior should be codified with a test one way or another.

    1. I have no idea what's going on here -- I just rearranged the code a bit. Since the comments say "be careful when you mess with this code or you'll break goal idea", I don't want to change it until I hear from @tejal or @fkorotkov.

    2. Do we still use goal idea? I thought we've deprecated that in favor of the IntelliJ plugin. But yeah, let's see what the experts on that area think.

    3. The comments actually say "Changing the behavior of this task may affect the IntelliJ Pants plugin..." - which is accurate - this is not used by the idea goal / IdeaGen task. That said, it seems to me splitting out a seperate undocumented goal dedicated to the IDEA plugin would be best for stability even if it introduces a bit of repetition. We should be able to freely edit the depmap goal without worrying about how it might affect the IDEA plugin.

    4. =you.

    5. The plugin only uses the --project-info output, so this change shouldn't affect the plugin.

  3. 
      
DT
PA
  1. 
      
  2. This should be isinstance(target, PythonTarget)... which of course is impossible since it would hairball the jvm backend. Why not just silently ignore targets that aren't Target or JvmTarget?

    1. I'm a bit confused here. PythonTarget subclasses Target. Do you mean just silently ignore non-JvmTargets? Or something else?

    2. I mean that the only two things you care about here are Target, formerly known as Dependencies, and JvmTarget. Python is the only thing being explicitly excluded because it was the only thing that used to exist outside of JVM targets and codegen. The reason we discourage checking against random other targets is because (1) it's an abstraction leak and (2) as more target types are introduced (e.g. C++) it becomes infeasible to rely on these types of checks.

      My primary complaint here is that the word "python" should not occur in a task that lives under src/python/pants/backend/jvm. I think the long term solution to this is to move this task out into some kind of ide backend that specifically depends on all of the other backends that it cares about. I guess just keep it in mind for a followup--a lot of this code is on the chopping block, at least from backends that must have clean dependencies.

  3. Parens are superfluous

    1. Parens make the precedence clear. I don't want to have to remember whether not has higher precedence than or.

    2. Stylistically I disagree, but I'm not passionate enough about it to make a fuss.

  4. 
      
NH
  1. 
      
  2. Could you collapse this and line 164 into a helper function? They both contain the same expression modulo DeMorgan's Law, and I think it'd raise the level of abstraction in a way that would make things clearer.

  3. 
      
DT
DT
DT
  1. ping? I think I addressed the review comments.

  2. 
      
TE
  1. Also confirming this does not affect the plugin.
  2. 
      
DT
Review request changed

Status: Closed (submitted)

Loading...