Make BaseGlobs.from_sources_field() work for sets and strings.

Review Request #3961 — Created May 31, 2016 and submitted

kwlzn
pants
3505, 3534
pants-reviews
ity, jsirois, peiyu, stuhood
  • Make BaseGlobs.from_sources_field() work for sets and strings.
  • Tests.

Fixes #3505.

verified this resolves the first two cases of #3505 in our monorepo.

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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
YU
  1. 
      
  2. If we want to enforce BUILD file syntax like "exclude=[xxx]" instead of "exclude=xxx", we want to make sure raw_excludes is a list. Otherwise it won't throw exception with the new change you made but what gets excluded will be bunch of single character strings.

  3. 
      
KW
YU
  1. 
      
  2. Do you know where is this method defined? I couldn't find in PantsRunIntegrationTest.

    1. ah, looks like R2 was a bad diff - fixed and fixed the contextmanager that let this slip through.

  3. 
      
KW
YU
  1. Ship It!
  2. 
      
PE
  1. would be good to also supply some failed examples, a few tests use TEST_BUILD and PantsRunIntegrationTest.file_renamed rename them to BUILD during tests

    1. the failure case/example is generated and inline in the unit test. seems to me like it's actually better to generate invalid BUILD files like this test does vs checking them in as arbitrary names and renaming them - as checked in copies of TEST_BUILD they hold no real meaning because you have to understand the rename paradigm to even utilize them.

      with generation, when you're trying to reason about the test it's much easier to look in a single code block to see the generated BUILD and test - vs having to reference two places, understand the temporarily renamed paradigm, etc.

      I'd almost like to see us move to purely generated for these sorts of tests for this reason - but it's open to larger debate well outside of the scope of this review.

    2. you are right, i missed there is one failure test, agree generating BUILD file is better

  2. doesn't seem we have a test case for this

    1. it's definitely tested - just in a roundabout/non-obvious way:

      [illuminati pants (kwlzn/engine/globs_types)]$ git diff
      diff --git a/src/python/pants/engine/legacy/structs.py b/src/python/pants/engine/legacy/structs.py
      index b3eb56d..1b2a6fd 100644
      --- a/src/python/pants/engine/legacy/structs.py
      +++ b/src/python/pants/engine/legacy/structs.py
      @@ -141,6 +141,7 @@ class BaseGlobs(AbstractClass):
           elif isinstance(sources, BaseGlobs):
             return sources
           elif isinstance(sources, string_types):
      +      raise AssertionError('this is tested!')
             return Files(sources)
           elif isinstance(sources, (set, list, tuple)):
             return Files(*sources)
      [illuminati pants (kwlzn/engine/globs_types)]$ ./pants test tests/python/pants_test/engine/legacy:filemap_integration
      ...
      13:03:07 00:00     [pytest]
      13:03:07 00:00       [run]
                           ============== test session starts ===============
                           platform darwin -- Python 2.7.10 -- py-1.4.31 -- pytest-2.6.4
                           plugins: timeout
                           collected 9 items 
      
                           tests/python/pants_test/engine/legacy/test_filemap_integration.py FFFFFFFFF
      ...
                           =========== 9 failed in 14.64 seconds ============
      ...
      13:03:23 00:16   [complete]
                     FAILURE
      
  3. 
      
ST
  1. 
      
  2. Does collections.Sequence not include list/tuple/set? Is there a generic type that does?

    1. no - afaict, the collections types seem rather limited/awkward. this is the rough mapping:

      [illuminati python2.7]$ grep ".register" _abcoll.py
      Iterable.register(str)
      Set.register(frozenset)
      MutableSet.register(set)
      MutableMapping.register(dict)
      Sequence.register(tuple)
      Sequence.register(basestring)
      Sequence.register(buffer)
      Sequence.register(xrange)
      MutableSequence.register(list)
      

      so if you match against collections.Sequence you can end up with all sorts of odds/ends - str, unicode, xrange, etc - but not set. not even generators, which are technically iterable sequences - except for the special cased xrange.

      thus, seemed more correct to focus on the 3 specific types we explicitly support. for general use tho, maybe it'd be useful to roll our own pants.util.generics?

  3. To defend against writing outside the buildroot, I think you'd want realpath.

  4. 
      
KW
PE
  1. Ship It!
  2. 
      
KW
Review request changed

Status: Closed (submitted)

Change Summary:

thanks gents! submitted @ b89fef8165e2a0195f42129092c6e0e325aa5322

Loading...