Polishing --ignore-patterns change

Review Request #3438 — Created Feb. 8, 2016 and submitted

tabishev
pants
tabishev/ignore_patterns_polish
2900
pants-reviews
benjyw, patricklaw, stuhood, zundel

In process of OSS pants adoption in Twitter I've discovered couple of problems in --ignore-patterns change.

This RB contain couple of fixes:
- do not access deprecated property in context.py
- do not pass explicitly spec_excludes in go_buildgen.py
- use --ignore-patterns option name in deprecation message for --spec-excludes

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

TA
MA
  1. I also have another bug to report, at least a deviation from the gitignore behavior. If a top-level folder is added to the ignore-files, it ignores anywhere else that folder name is found.

    So if I have log in the ignore-files, it will ignore the BUILD files in com/foursquare/target/module/log/BUILD. I can add log to the .gitignore file and still have version controlled files under that subdir.

    1. I've created two BUILD files in log dirs: log/BUILD and tmp/log/BUILD. git check-ignore log tmp/log output is empty before any changes to .gitignore, and contains both BUILD files after adding log to root .gitignore. So this behaviour is consistent with .gitignore behaviour. To ignore only root log folder you need to use /log syntax.

    2. So Timur and I talked offline and his implementation does indeed match the gitignore spec. The gitignore has an extra bit compared to the Pants ignore_patterns, in that it will override the gitignore if a match is already under version control.

      Changing to the ignore_patterns has a bit of a gotcha in that if people have lots of spec_exclude entries in their big repos, the chances increase that they will accidentally be excluding targets that previously had been scanned once they convert to patterns, unless they are careful when converting.

      My solution was to prepend all top-level files/folders explicitely with '%(buildroot)s/X'. Timur pointed out that you can get the same effect from /X'.

  2. I fell victim to this. It would probably mean going through a deprecation cycle but I would prefer the fix go the other way around, i.e. change the flag to be build_file_ignore_patterns or build_ignore_patterns.

    Just having ignore_patterns in the default scope, it is not clear to the uninitiated what it refers to.

    1. Damn it =( Sorry about that.

      If we will change option name it will be under deprecation cycle too, also there was a big conversation about name of the flag, and I will prefer to stick with current name.
      Both spec_excludes and ignore_patterns are advanced options, but maybe it's a good idea to make them in default scope.

  3. 
      
TA
ST
  1. Ship It!
    1. ...although it would be good to add at least one coverage test. Maybe for scan?

    2. I've added test on Context#scan + build_ignore_patterns, not sure the place is good... I don't think it's reasonable to add test on scan + spec_excludes too as option is deprecated and it implies to add spec_excludes parameter to BaseTest#context method.

    3. It was simpler than I thought so I've put test on scan + spec_excludes too. It's hacky, but I think it's better than nothing here.

  2. 
      
MA
  1. 
      
  2. 
      
TA
TA
BE
  1. 
      
  2. Why is this the right thing to do?

    1. Because all knowledge about what to ignore already contains in address_mapper instance itself.

  3. 
      
TA
TA
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 6ba5c33b566be010073593a463fdae617d1bbeb6

PA
  1. Ship It!
  2. 
      
Loading...