Fix hardcoded pants ignore from 'dist/' to '/rel_distdir/'. Use pants_ignore: +[...] in pants.ini

Review Request #3927 — Created May 24, 2016 and submitted

wisechengyi
pants
3495
pants-reviews
benjyw, ity, jsirois, kwlzn, stuhood, zundel

Earlier dist/ in pants ignore inadvertently ignores a/b/c/dist as well, so this RB corrects so it will only ignore root level /dist and its subdirs.

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

KW
  1. 
      
  2. 1) should this actually be /dist/ or does the trailing slash not matter? e.g. would that not also exclude ./dist itself (and ./fruit in your test)?

    2) could you duplicate the dist entry in the pants-ignore of pants' pants.ini? I think this default is currently being overwritten.

    1. yeah, this should be /dist/

    2. corrected to /distdir/, and test added. thanks!

  3. 
      
ZU
  1. Ship It!
  2. 
      
JS
  1. 
      
  2. This should use `register.bootstrap.pants_distdir` (see `register_bootstrap_options` pydoc above) to derive the dir to use in the ignore list so a user doesn't need to both customized `--pants-distdir` _and_ this option when they simply want to change the dist dir.
    1. corrected. thanks!

  3. 
      
WI
JS
  1. The last 2 comments are not strong opinions, I'm more concerned about the 1st.
  2. What is the motivation for changing the default, ie nuking .*? If none/unintentional, please nuke it from pants' own pants.ini instead.

    1. Yeah I was hesitating which location .* should go...

      Moved it to source. thanks!

  3. I'm not sure this is a useful test - it tests the contract of .gitignore. Granted you were unawre of that contract, but that seems out of scope to me.

    1. I see your point, although I hope it could bring more clarity for folks joining in the future.

  4. Ditto.  Seems to me simply testing that `dist_dir` and default excludes are linked is exactly the test you want.
    1. good point. integration tests added.

  5. 
      
WI
KW
  1. 
      
  2. pants.ini (Diff revision 3)
     
     

    should this get anchored as well? e.g.

    /build-support/*.venv/

  3. 
      
WI
WI
Review request changed

Status: Closed (submitted)

Change Summary:

8021eec0aece0317ea442d9c358afba05e1d3b1d thanks gents!

Loading...