BuildFileManipulator implementation and tests

Review Request #977 — Created Sept. 2, 2014 and submitted

benjyw, jsirois, zundel

Add BuildFileManipulator implementation and tests to contrib

The BuildFileManipulator is an effectively dependency-free utility for
manipulating target dependencies in BUILD files programmatically.
It is one of the key tools that enables buildgen, so it makes sense
for it to live in the buildgen contrib directory as the first of many
buildgen components to go upstream.

New tests written, final CI is green at

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
  1. for a non trivial amount of parsing logic created, do we have an github issue or doc/email or the class python doc somewhere describing what it is?

    1. Thanks for the review, Eric. I'll definitely be following up on all of these things, but I have a backlog of feature work internally right now. I just wanted to clarify that I don't necessarily intend to merge this CR soon, but since this is code we're using on our fork internally that I intended to be open source, I at least wanted to put it out there somewhere in case there was outside interest.

      In other words--it might be a while before I clean this stuff up and actually submit. Luckily this is effectively dependency free code, so it won't rot too much.

  2. src/python/pants/base/BUILD (Diff revision 1)

    I don't see a dependency on build_file (no imports)

  3. As an overall comment, I'll say that you have an AST here, you might want to use the structure of the tree to your advantage. It is pretty common to use Visitor pattern in a parser.

    1. I don't think a Visitor pattern is really appropriate here, primarily because I am always looking exactly 1 and 2 layers deep for my nodes of interest, and it is highly structured (and not recursive). It would be nice to have some sort of selector syntax for this, but it seems like overkill, and in any case this is all complicated by needing to be aware of comments in and around certain parts, which are thrown away in the AST.

  4. Add a unit test for this and other cases where exceptions are raised.

    1. Most of them are tested. I might have missed this one, but it's also kind of a crazy edge case (you have a target that doesn't have a name param--it's already a parse error 100% of the time in all other use cases). I'm going to punt on more testing--the unit testing coverage is pretty good and demonstrates all of the major ways this can go wrong.

  5. idea: since you have a line number available, you could you could reference/copy the _format_context_msg() printing code in for most if not all of these exceptions.

  6. throw us a bone here: at a minimum add docstrings here, at the class level, and to api methods. Also, it looks like you don't intend for end users to use this constructor but instead use load() as a factory method instead, make a note of that.

  7. My personal preference would be to ditch the generator here. It is more code than just appending to a list and returning it.

    1. I'm about 50/50 on doing this vs breaking the whole thing into an if/else for the empty case. So I'm just going to leave it, in the interest of doing less unnecessary work and not risking subtle breakage.

  8. nit: Is this diff stuff all debugging? Regardless, you could re-write all this into a buffer and then dump to stderr all at once. Individual calls to stderr might get other stuff interleaved if there is threading or background processing going on.

    1. Good point, but I'm also going to punt this. In practice I've never seen this happen, and I have a limited amount of time to do this cleanup.

  9. s/sentinal/sentinel/

  10. +1. Also, I agree with your embedded comment: this test covers a lot of ground, it might be easier to differentiate the different failure modes in smaller tests.

    1. I'm going to punt on this, especially now that this is going to live in contrib. I'd like to get to it eventually, but right now this class is very heavily battle tested, and the unit test coverage here is pretty decent.

  11. s/sentinal/sentinel/

  12. nit: get rid of pytest import and use with self.assertRaises()

  1. Ship It!
  1. Ship It!
  1. Other than a few minor issues below

    1. Nothing to register, right now this is just library code. It doesn't export tasks, targets, etc.

  2. nit date (but maybe this is a file youve published before?)

    1. It technically has been seen on the internet before this, since this CR started in 2014.

    1. As in, I should just inline it? I prefer not to do computation within calls to format if I can avoid it.

    2. oops, I was looking at the wrong one. Fixing.

    3. You define indent_spaces here and never use it. I think you are referring to the use in indented_lines() above of a similarly named variable.

  3. I'm not sure what all this is about? Some debuggging you want to save for later? Add a comment to justify or remove it.

  4. You have this one commented out, I'm not sure how valid this test is if a missing target passes. Maybe you need an exception type (or examine the text in the assertion message) to differentiate between a target that isn't found and one that has parse errors?

    1. Agreed that these should have different exception types and this should all be broken out, but as I mentioned above, I'm going to punt on that for now. I'm going to comment out this target name to make it clear that it isn't a valid test though, and add a TODO.

  1. Seems like its in pretty good shape for a contrib upload. Later you might want to come back with a README.

Review request changed

Status: Closed (submitted)

Change Summary:

Thanks for the thorough review, Eric. Upstream @ bda64ee1cece6a263f720d77bf2e57874020db0e