Add test for jar_publish. This required some refactoring so that functions could be mocked.

Review Request #1082 — Created Sept. 26, 2014 and submitted

dturner-tw
pants
a81daba...
pants-reviews
ity, jsirois, stuhood

Includes a few minor cleanups to comments, error messages, docstrings,
etc. This is in preparation for some improvements to how the scm integration is done.

Ran the new tests and the integration tests.

  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
DT
ST
  1. 
      
  2. Thanks for breaking these out. But method docs for each of these would be good.

  3. formatting

  4. Hm... can you explain this change?

    1. self.force is never true here, because the preceding if statement says "if ... not self.force".

  5. tests/python/pants_test/base_test.py (Diff revision 2)
     
     

    This needs a comma after it.

  6. 
      
DT
ST
  1. Works for me, thanks!

  2. particular

  3. 
      
PA
  1. 
      
  2. self._props = props or OrderedDict()

  3. tests/python/pants_test/base_test.py (Diff revision 3)
     
     

    The pants wrapper here is deprecated, might as well take the opportunity to clean this up (it should just be newline+comma delimited strings of specs)

    1. removed pants wrapper.

  4. 
      
IT
  1. 
      
  2. jvm_args is being lost after this afaics. maybe you wanted to update self._jvmargs finally?

  3. nit: this could have an early exit instead of the whole method in an if block.
    if not repo.get('auth')

  4. this will throw if there is no key 'resolver' - repo.get('resolver')

  5. 
      
DT
DT
JS
  1. Thanks for the refactor - 1 bug fwict.

  2. The repo argument is no longer a Target but instead an exported object. You'll just need export this up in alias_groups above and use the alias as a symbol, ie: artifact(..., repo=internal)

  3. 
      
DT
IT
  1. run checkstyle on this - should help you fix all the indent etc issues throughout

    1. I ran checkstyle and didn't see indentation issues other than the one badly-formatted comment. I did see a few other issues, though.

  2. repo.get - use .get(..) for map access throughout

    1. I disagree. It is correct to use .get when None is an acceptable result. In this case, the resolver is going to be passed as a command-line option. So we would get an error when we went to use it, and that error would be less clear than KeyError (since it could be caused by either a missing key or an entry with value None).

  3. %s/#One/# One

  4. 
      
DT
JS
  1. Ship It!

  2. 
      
JS
  1. In master @ https://github.com/pantsbuild/pants/commit/fb2f37f42e3dfce10512a4bf02018c32b508ebb7
    Please mark this review as submitted.

  2. 
      
DT
Review request changed

Status: Closed (submitted)

Loading...