Make the name= target keyword optional in BUILD files.

Review Request #4275 — Created Oct. 1, 2016 and submitted

benjyw
pants
pants-reviews
kwlzn, stuhood, zundel
If omitted, it defaults to the name of the directory, which is
a nice, boilerplate-reducing convenience in support of a very
common idiom.

If omitted from two targets in the same BUILD family, you get the
error you expect, namely that two targets have the same name.

Note that Target instances still always requires a name; It's just
the machinery that creates them from BUILD files (both in the old
and new engine) that substitutes the default.  Targets created
in other ways (e.g., synthetically) must be provided with an
explicit name.

Adds tests for this, and also removes the names from a couple of
our example BUILD files, as added proof that it works.

Also fixes some of the build file parser exception tests:
previously they did throw exceptions, but not for the reason
the test author intended. Now we check the exception message
to be sure the failure is the one we meant.

CI passes: https://travis-ci.org/pantsbuild/pants/builds/164758047

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
ZU
  1. Ship It!
  2. I like the new feature, but do we want to make this the cannonical example without comment?

    This might be confusing for users in our environment where we still routinely use multiple targets in BUILD files.

    1. I'll add a comment explaining this.

  3. 
      
MA
  1. It would be nice to supply a custom error for case where two targets elide the name attribute, that is really the only gotcha I can think of here.

    1. I don't see a straightforward way to do that.

  2. Might as well delete the commented code here.

  3. 
      
ST
  1. 
      
  2. src/python/pants/engine/legacy/parser.py (Diff revision 1)
     
     
     

    Memoizing the ParseContext per-rel_path is going to be very expensive (note that his method is @memoized_method)... it's used exactly once.

    Is it strictly necessary to move back to recreating the ParseContext per parsed file? I began mutating it because it avoids a lot of setup here. If so, it might be necessary to split out the per-path symbols (ie, those with a reference to the ParseContext) to get the correct behaviour there.

    1. Hmmm, I didn't understand why the ParseContext was initialized with no rel_path, and then modified later. If the reason was performance then that should have been clearly documented...

      Will figure out some other solution.

  3. 
      
NH
  1. 
      
  2. What happens if spec_path is ''? Should we default to ''? Please add a test for this case.

    1. Good point. It now detects this case and errors since, as you say, defaulting to the name of the repo dir itself is not consistent.

  3. Can we assume filepath here is relative? If it's not and the BUILD file's relative path to the build root is '', the default name will be the containing directory, which would be confusing. Especially if an org doesn't require that it have a particular name.

  4. 
      
BE
BE
ST
  1. Thanks!

  2. src/python/pants/engine/legacy/parser.py (Diff revision 2)
     
     
     
     
     
     

    Great, much clearer.

  3. 
      
BE
NH
  1. Ship It!
  2. 
      
BE
BE
Review request changed

Status: Closed (submitted)

Change Summary:

ac0fe8b7ae0e8d57370b0f14a177f3a7836fdd9a

Loading...