Adding a flexible directory re-mapper for the bundle

Review Request #1181 — Created Oct. 17, 2014 and submitted

wonlay
pants
701
99ddae0...
pants-reviews
benjyw, dturner-tw, ity, patricklaw, zundel

We're currently using this convenient DirectoryReMapper in the search team to organize the dir structure of the bundles. Also seeing other team is also doing similar things. It's a quite general requirement.

Had a meeting with Ity, and since we're asked to remove all the custom functions in our BUILD files, it's better to get this convenient mapper into pants itself.

PANTS_DEV=1 ./pants goal test tests/python/pants_test:all
./build-support/bin/ci.sh

  • 0
  • 0
  • 5
  • 1
  • 6
Description From Last Updated
DT
  1. 
      
  2. Why is this in jvm_binary? Could it be useful for other target types?

    1. it's for the "class Bundle(object):" within the jvm_binary, only this bundle has the mapper, so i put it here.

      do you have any better location for it?

    2. No, that's good; there might be an equivalent possible for python pexes, but apparently that's a totally different codepath, so we don't need to care.

  3. Instead of removing this, why not change the assert to self.assertEquals(app.bundles[0].filemap[fivexml_key], 'config/five.xml')

    1. good idea! changed to use that key.

  4. 
      
WO
DT
  1. Ship It!

  2. 
      
PE
  1. 
      
  2. also relative to get_buildroot()

    1. the dest is actually not relative to get_buildroot(), it's the raw path in the output bundle, does not related to anything

    2. i see, thanks for clarifying

  3. still new to the convention here, this may deserve a test

    1. added a test for this

  4. 
      
JI
  1. Ship It!

  2. 
      
IT
  1. please add patrick,benjy, eric to this as well.

    1. Usernames are patlaw, benjyw, and zundel.

    2. er, patricklaw

    3. added those reviewers

  2. 
      
ZU
  1. 
      
  2. Is there a reason not to declare an alias for DirectoryReMapper in register.py? Even if you don't want to provide a different name, I think this helps us get it in the Build Dictionary public documentation - see build_manual.py

    1. added to register.py and removed the import in the tests

  3. 
      
WO
PE
  1. Ship It!

  2. 
      
BE
  1. Ship it after changing the param name as mentioned below.

  2. file is a global symbol in python that you're shading here. Call it 'path' (which is in fact more precise anyway, as this is a string not a file handle).

    1. renamed, thanks

  3. 
      
WO
ZU
  1. It would be great if you could run this through travis-ci. There are some instructions here: http://pantsbuild.github.io/howto_develop.html

    Basically you need to fork the pantsbuild/pants repo if you haven't already, then push your branch to your fork. Then, visit the pantsbuild/pants repo and create a Pull Request from your branch. Travis-ci will kick off automatically. Record your branch name and the PR number in the RBcommons fields above.

    1. and created a pull request for the ci job: https://github.com/pantsbuild/pants/pull/701

    2. Great! can you enter '701' in the 'Bugs' field in this Review Request above?

    3. added

    4. the ci job passed on: https://github.com/pantsbuild/pants/pull/701

  2. nit: I like to create a different exception class for each exception I raise to make testing more accurate. For example, if you were to add a unit test for instantiating DirectoryMapper() directly, you wouldn't be able to tell with

    with self.assertRaises(ValueError):

    if the ValueError raised was from your test, or if it was from some other problem in the code.

    nit: also use .format() style string formatting in new code.

    1. added a custom error and changed to use .format

  3. nit: use .format() style string formatting.

  4. nit: I like your existing test, but you could more directly test the condition and explicit raise if: 1) the raise() for missing dest used a custom exception type and 2) if you instantiated DirectoryReMapper() directly in the unit test. The AddressLookupError is a base class used by about 8 different exceptions and is several re-raises removed from the actual root cause.

    1. added a direct creating test

  5. 
      
PA
IT
  1. almost there!

  2. All these test methods do the almost the same setup for the first 10-15 lines at least - please extract out the boilerplate

    1. just tried get a method to setup this, only the dir/file structure part is actually similar, but since most of the tests use different dir structures, the code looks even worse. reversed them back.

  3. +1 Eric's comment.

    s/app = self./self.

  4. s/test_bundle_add_add/test_bundle_add

  5. 
      
WO
WO
ZU
  1. Still LGTM

  2. 
      
WO
DT
  1. This is now in master at 7b39eac93d839acd0cc3f9123688a01e3a20ae3d. Please close this rb as 'submitted'. Thanks for contributing.

    1. thanks! closing

  2. 
      
WO
Review request changed

Status: Closed (submitted)

Loading...