Make `SCMTool.get_file()` take `base_commit_id`.

Review Request #7265 — Created April 29, 2015 and submitted

smacleod
Review Board
release-2.0.x
7264
f2fc51f...
reviewboard

All SCMTool implementations should now take keyword arguments and a
base_commit_id argument. All internal implementations have been
converted and third party tools will cause a deprecation warning if
they do not have the proper method signature.

The HgTool now takes advantage of this `base_commit_id` argument when
fetching files (since hg git diffs do not include index lines which
we use to determine what revisions to fetch). If `base_commit_id` is
provided it will override anything parsed from the diff.

The same applies to SCMTool.file_exists()

New tests, unit tests pass, posted hg diff now uses the provided base_commit_id.

Description From Last Updated

're' imported but unused

reviewbotreviewbot

Single quotes.

brenniebrennie

Single quotes.

brenniebrennie

'FileNotFoundError' imported but unused

reviewbotreviewbot

No need for parens here.

chipx86chipx86

local variable 'new_changeset_id' is assigned to but never used

reviewbotreviewbot

local variable 'orig_changeset_id' is assigned to but never used

reviewbotreviewbot

Col: 17 E221 multiple spaces before operator

reviewbotreviewbot

Col: 21 E221 multiple spaces before operator

reviewbotreviewbot

We could probably make this a utility function, cutting down this code a lot and making it more reusable. Another ...

chipx86chipx86

Col: 17 E221 multiple spaces before operator

reviewbotreviewbot

Col: 17 E221 multiple spaces before operator

reviewbotreviewbot

'warnings' imported but unused

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. reviewboard/scmtools/hg.py (Diff revision 1)
     
     
     're' imported but unused
    
  3. reviewboard/scmtools/models.py (Diff revision 1)
     
     
     'FileNotFoundError' imported but unused
    
  4. 
      
brennie
  1. Looks good to me!

  2. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Single quotes.

  3. reviewboard/scmtools/hg.py (Diff revision 1)
     
     

    Single quotes.

  4. 
      
chipx86
  1. Should probably have unit tests for the warning behavior and changes in the hg handling of the parameters.

    Also, I imagine we need this for file_exists as well.

  2. reviewboard/scmtools/models.py (Diff revision 1)
     
     

    No need for parens here.

  3. 
      
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. reviewboard/scmtools/hg.py (Diff revision 2)
     
     
     local variable 'new_changeset_id' is assigned to but never used
    
  3. reviewboard/scmtools/hg.py (Diff revision 2)
     
     
     local variable 'orig_changeset_id' is assigned to but never used
    
  4. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. reviewboard/scmtools/models.py (Diff revision 2)
     
     
    Col: 17
     E221 multiple spaces before operator
    
  3. reviewboard/scmtools/models.py (Diff revision 2)
     
     
    Col: 21
     E221 multiple spaces before operator
    
  4. 
      
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/scmtools/tests.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     

    We could probably make this a utility function, cutting down this code a lot and making it more reusable.

    Another interesting idea is adding an assertWarns in the base TestCase.

    1. So, my preferred fix would be to have basically this on reviewboard.testing.testcase:TestCase:

      from contextlib import contextmanager
      
      
      ...
      
      
      @contextmanager
      def assert_warns(self, cls=DeprecationWarning, message=None):
          with warnings.catch_warnings(...) as w:
              # Stop filtering ...
      
              yield
      
              # asserts...
      
              if message is not None:
                  # that assert.
      

      The callers then get to be very slim and we don't have to worry about all that boilerplate.

    2. How do you think we should compare the messages? Currently I've been using .startswith(...) because that works with my particular case - is that fine here? Would you like to use .find() instead, or something else?

    3. How hard would it be to have a full-on equality check? If the string is predictable, that might just be the way to go.

  3. 
      
SM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. reviewboard/scmtools/tests.py (Diff revision 4)
     
     
    Col: 17
     E221 multiple spaces before operator
    
  3. reviewboard/scmtools/tests.py (Diff revision 4)
     
     
    Col: 17
     E221 multiple spaces before operator
    
  4. 
      
chipx86
  1. Ship It!
  2. 
      
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. reviewboard/scmtools/tests.py (Diff revision 5)
     
     
     'warnings' imported but unused
    
  3. 
      
SM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/scmtools/clearcase.py
        reviewboard/scmtools/mtn.py
        reviewboard/scmtools/git.py
        reviewboard/scmtools/hg.py
        reviewboard/scmtools/localfile.py
        reviewboard/scmtools/core.py
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/bzr.py
        reviewboard/scmtools/cvs.py
        reviewboard/scmtools/plastic.py
        reviewboard/scmtools/tests.py
        reviewboard/testing/scmtool.py
        reviewboard/scmtools/svn/__init__.py
        reviewboard/scmtools/models.py
    
    
  2. 
      
SM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (6b34af1)
Loading...