Shorten too long target ids by adding a hash.

Review Request #2894 — Created Sept. 25, 2015 and submitted

wisechengyi
pants
2276
1182fc9...
pants-reviews
ity, molsen, nhoward_tw, stuhood

Shorten too long target ids by adding a hash. target id is often used to generate filename which cannot be longer than 255 characters in unix system, therefore a 200-character limit is chosen as a safe measure by replacing the partial string with a hash.

Two unit tests added.

CI: https://travis-ci.org/pantsbuild/pants/builds/82283604

  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
MO
  1. 
      
  2. src/python/pants/base/target.py (Diff revision 1)
     
     

    Can we update the docstring to indicate what the actual limitations are so the magic numbers are clearer.

    My concern is that someone may break something in the future if its not clear what the actual limitations are.

  3. src/python/pants/base/target.py (Diff revision 1)
     
     

    Please don't use unicode it is deprecated in python 3. You can use six.text_type

    https://pythonhosted.org/six/#constants

    1. used str.format instead

  4. 
      
NH
  1. I like the approach, it's a good start. I've got a few suggestions.

    • Could you update the summary to reflect the change more directly? It's what ends up in the changelog, so something more like 'Shorten too long target ids by adding a hash'.
    • Please add tests for this.
    • Maybe update the pydoc for 'id' and 'identity' so that it notes the the hashing.
  2. src/python/pants/base/target.py (Diff revision 1)
     
     

    Pants codebase uses str.format() for creating strings, not '+'. Here, though, I would suggest using '.'.join(<head>, <hash>, <tail>)

  3. 
      
IT
  1. please change the subject for this review to something that actually says what you are doing

    1. +1

      Other than that, this looks good.

  2. 
      
WI
MO
  1. 
      
  2. I think you would be better off testing equality because you are more likely to catch off by one errors in either direction. You should be able to predict exactly what the length is so just check for that.

  3. I would rather not see so many asserts in the test. As a general rule 1 assert per test means that a if a test blows up you know what will fail in that test. If you have 4 asserts in your test then you might only know about one fail case.

  4. 
      
WI
MO
  1. thanks for splitting up the tests... once you change the title LGTM

  2. 
      
WI
WI
WI
MO
  1. Thanks for updating the Summary

  2. 
      
WI
ST
  1. @Yi: the CI is red... please get that green, and then I will merge this.

  2. 
      
WI
WI
Review request changed

Change Summary:

updated CI

Testing Done:

   

Two unit tests added.

   
~  

CI: https://travis-ci.org/pantsbuild/pants/builds/82280401

  ~

CI: https://travis-ci.org/pantsbuild/pants/builds/82283604

Loading...