Migrate dependencies, depmap, detect_duplicates and check_published_deps to new options

Review Request #1269 — Created Oct. 31, 2014 and submitted

ity
pants
4229140...
pants-reviews
benjyw, dturner-tw, jsirois, zundel

Migrate dependencies, depmap, detect_duplicates and check_published_deps to new options

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

  • 0
  • 0
  • 8
  • 0
  • 8
Description From Last Updated
JS
  1. 
      
  2. s/dependencies_is_internal_only/dependencies_is_external_only/

  3. s/depmap_is_minimal/depmap_is_external_only/

  4. 
      
BE
  1. 
      
  2. Single quotes.

  3. Why add the 'is' prefix? Doesn't --internal-only, --external-only seem more apropros?

  4. This is broken - self.internal_only_flag and self.external_only_flag no longer exist.

    However they were only needed in the past to get the full (scope-qualified) flag name in the error message. But now you can just hard-code --internal-only and --external-only in the message string. In the new-style flags world there's no need for those long scope-qualified flags.

  5. Ditto here. --internal-only, --external-only?

  6. 'store' is the default action, so we can omit it.

  7. remove unnecessary parentheses.

  8. Delete this line. cls is no longer used.

  9. This is wrong - instead of flag names you're interpolating flag values (True/False) into the string.

    But as above, you can simply hard-code the unqualified flag names here.

  10. Note that as a result of switching that flag to 'append', plus this line, it is now impossible to specify no excludes at all. Is that OK?

  11. Thanks for doing these! See comments inline.

ZU
  1. 
      
  2. I'm not so sure I like adding the -is prefix to the flag. Makes sense for the variable, but not so much from the command line.

  3. typo, dependencies_is_external_only

  4. I don't think this is going to execute correctly - you removed cls.external_only_flag and cls.internal_only_flag.

  5. same comment on the 'is' prefix

  6. typo, depmap_is_external_only

  7. this is going to display "At most one of True or True can be selected"

  8. lost the defaults on this option.

  9. 
      
IT
BE
  1. 
      
  2. This condition can now never be true. Any flag values will be appended to the default, so excludes will always contain, at least, EXCLUDED_FILES.

    1. aha, thats true - removed it.

  3. 
      
IT
BE
  1. Ship It!

    1. Ping? When do you plan to submit this?

    2. Its on master now.

    3. Yay! Thanks. We're really close now. One task left! (jar_create).

      I can do it today if you won't be able to get to it.

    4. not sure how I missed that one, should be out soonish

    5. Looks like the detect duplicates change broke its unit test?

  2. 
      
ZU
  1. Ship It!

  2. 
      
IT
Review request changed

Status: Closed (submitted)

Loading...