BuildFile refactoring: add more constraints to BuildFile constructor

Review Request #3376 — Created Jan. 26, 2016 and submitted

tabishev
pants
tabishev/build_file_strict_mode
2838
pants-reviews
dturner-tw, patricklaw, stuhood, zundel

BuildFile.__init__ used to try to find any BUILD file in a passed directory and use it. Now it requires that an explicit (existing) BUILD file is passed in order to succeed.

Previous behaviour doesn't work well with future pants_build_ignore option because this option can forbid some BUILD files extensions.

This RB adds the constraint to BuildFile constructor: input path should be a path to an existing BUILD file only.

https://travis-ci.org/ttim/pants/builds/105066148

  • 0
  • 0
  • 7
  • 0
  • 7
Description From Last Updated
TA
TA
TA
TA
TA
TA
ZU
  1. 
      
  2. src/python/pants/base/build_file.py (Diff revision 5)
     
     

    I'm not sure users who see this error message will understand what to do. If it must be true, then why is it an option? You still support the code path for must_exist=False, are you planning to remove it? If so, then say so.

    1. I've updated warning, is it ok? Do we have other ways to show this kind of warnings?

  3. 
      
PA
  1. Can the renaming be separated from the actual change the review is intended to enforce?

    1. Plan was to make them in separate diffs, just to make little bit efforts on creating PR&etc. But I agree better to make them separately. So I've removed renaming from this change.

  2. 
      
TA
TA
ZU
  1. 
      
  2. why did you remove the hint?

    1. Because I've removed substitution method from the class. Reason to removal - we don't want to expose BuildFiles from address_mapper as much as possible. Reason to remove, not to deprecate - method was introduced after last release (PR was merged right after release).

  3. thanks for this test

  4. 
      
TA
PA
  1. 
      
  2. src/python/pants/base/build_file.py (Diff revision 6)
     
     

    Personally I would use double quotes if my string needs a single quote--but no big deal either way.

  3. src/python/pants/base/build_file.py (Diff revision 6)
     
     

    Space before BuildFile -- implicit string concat doesn't add spaces.

  4. src/python/pants/base/build_file.py (Diff revision 6)
     
     

    Ditto space

  5. src/python/pants/bin/goal_runner.py (Diff revision 6)
     
     

    Out of curiosity, is it actually ./? I would think it would be //spec_path:target. But this is in the hazy past for me.

    1. Both of them will work. We need ./ for distinguish with goal and we normalize all paths so ./test == test in terms of targets, but != in terms of goals.

  6. "because that BUILD files ..." grammar. Either "because that BUILD file ..." or perhaps "Because that directory contains no BUILD files defining..."

  7. I'm confused about the need for differentiating these two cases. Can you clarify? Regardless of 1 vs N options, we should either be explicit about the spec or always make the spec relative. Am I missing something?

    1. This part about source file for different targets, changed to ":target (from BUILD.tools)" syntax.

  8. Just want to double check that there's some test coverage on this. Exception creation is notorious for breaking in the worst way only after users hit it.

    I mention this because at the very least, spec_path isn't needed in the interpolation template.

    1. Last one uses was_not_found_message which includes spec_path. The spec_path variable was from old code, I've removed it now, thanks!

  9. This should return the appropriate type of Address--that is, it should return a BuildFileAddress when it's possible to, as the old one did.

  10. 
      
TA
PA
  1. Ship It!
  2. 
      
TA
TA
TA
Review request changed

Status: Closed (submitted)

Change Summary:

c88c53784a10d33065adb895724d76afae8289b9

Loading...