Add Support for "exclude" to globs in BUILD files

Review Request #3828 — Created May 5, 2016 and submitted

yujiec
pants
3228, 3338
pants-reviews
jsirois, stuhood, zundel

New Engine has to support "exclude" feature in globs to allow us to parse all BUILD files in the pantsbuild/pants repo.

Changes made:
1. in BaseGlobs class, handle "exclude" argument
2. LazyFileContent now takes 2 pathglobs, 1 for matching globs, 1 for excluding globs.
3. "exclude" logic is added to LazyFileContent class.
4. add a testproject "file_set" under testprojects/tests/python/pants/file_sets/. It has 1 BUILD file and multiple empty \*.py files.
5. add new tests in engine/exp/legacy/test_filemap_integration
6. fix a bug in apply_path_wildcard and apply_path_dir_wildcard in fs.py. fnmatch should take basename of a path.

Things to note:
1. Nesting exclude is supported. (ie: exclude=\[globs('...', exclude=\[...\])\])

https://travis-ci.org/pantsbuild/pants/builds/128960483

  • 0
  • 0
  • 2
  • 1
  • 3
Description From Last Updated
ST
  1. 
      
  2. src/python/pants/engine/exp/fs.py (Diff revision 1)
     
     

    Please move comments to the line before the code they're commenting on... when they're at the end of a line like this it's difficult to use complete sentences.

    1. Stu, I am sorry I didn't make this clear, but I didn't mean for it to be a comment.
      actually I want to ask you this: will d.path ends with '/'? I believe it won't but just want to make sure because a trailing slash will make basename output to be empty string.

    2. No, I don't think it will. But you can trust that the inputs are definitely directories in this context, so you could safely append the slash if it isn't there already.

    3. I actually want to do the opposite - remove the trailing slash.
      basename('xxx/yyy') = 'yyy' but basename('xxx/yyy/') = ''.

      But since d.path won't end with '/' I think it is good now.

  3. Can remove the TODO here: I think any other kwarg should be unsupported for now.

  4. src/python/pants/engine/exp/legacy/globs.py (Diff revision 1)
     
     
     

    should use:

    excluded_patterns.extend(exclude)
    

    ... rather than the for loop.

  5. excluded.value will likely be a list/tuple, so this will run in linear time.

    Recommend converting the excluded.value to a set before this comprehension.

    1. done.

    2. oops, should take set out of comprehension.

    3. done.

  6. src/python/pants/source/wrapped_globs.py (Diff revision 1)
     
     
     

    since this doesn't use the cls reference, you can make it a static method:

    @staticmethod
    def process_raw_excludes(raw_excludes):
    
  7. Excellent! Would be good in general to break this into more, smaller tests... that generally helps with debuggability.

  8. 
      
YU
YU
YU
YU
ST
  1. Thanks!

    Please add some additional reviewers... maybe @zundel and @jsirois

    1. Also, would you mind adding a testcase to highlight that item 6 is fixed? Something like this would work:

      self.assert_walk(Files, ['**/3.t*t'], ['a/3.txt'])
      

      (see https://rbcommons.com/s/twitter/r/3840/)

    2. done.

  2. 
      
WI
  1. 
      
  2. This issue has been closed. Please remove it from docstring.

  3. src/python/pants/source/wrapped_globs.py (Diff revision 3)
     
     
     
     
     
     
     
     

    from twitter.common.collections import maybe_list should already do so.

    >>> from twitter.common.collections import maybe_list
    >>> 
    >>> maybe_list('123')
    ['123']
    >>> maybe_list(['123'])
    ['123']
    
    1. I don't think it fits the need here. Element can be string, list of string and also globs object. In case of globs object, maybe_list will throw exception.

    2. maybe_list will apply list conversion for all expected_types, while in this case, for 2 expected types, string - we want to convert it to list, baseglob - we don't.
      Extra logic will need to be applied if we want to use maybe_list which is not worth it. Thus drop the issue.

  4. Might be better to obtain TEST_EXCLUDE_FILES by scanning all the files under testprojects/tests/python/pants/file_sets/, and do an assert to make sure there are no discrepencies/surprises.

    1. are you suggesting adding a new test case to check whether os.walk gives the same result as TEXT_EXCLUDE_FILES?

    2. Yes, because for example if someone else wants adds some files to the test set, it may cause all the tests to fail because some are not expected. Asserting on TEXT_EXCLUDE_FILES (better at setup stage if all tests are independent) could help send a clear signal that these tests are expecting this set of files. It is not an issue for 1 or 2 files, but it is hard to tell in this case just by eyeballing.

    3. makes sense.
      done.

  5. 
      
YU
ZU
  1. I looked over the tests and they look good to me.

  2. 
      
YU
WI
  1. Thanks!

  2. 
      
YU
YU
YU
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as a5edfe0b335c6bb59feea50fc6822c70cc76a880

Loading...