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

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

Information

Review Board
release-2.0.x
f2fc51f...

Reviewers

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)
     
     
    Show all issues
     're' imported but unused
    
  3. reviewboard/scmtools/models.py (Diff revision 1)
     
     
    Show all issues
     'FileNotFoundError' imported but unused
    
  4. 
      
brennie
  1. Looks good to me!

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

    Single quotes.

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues
     local variable 'new_changeset_id' is assigned to but never used
    
  3. reviewboard/scmtools/hg.py (Diff revision 2)
     
     
    Show all issues
     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)
     
     
    Show all issues
    Col: 17
     E221 multiple spaces before operator
    
  3. reviewboard/scmtools/models.py (Diff revision 2)
     
     
    Show all issues
    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)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues
    Col: 17
     E221 multiple spaces before operator
    
  3. reviewboard/scmtools/tests.py (Diff revision 4)
     
     
    Show all issues
    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)
     
     
    Show all issues
     '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:
Completed
Change Summary:
Pushed to release-2.0.x (6b34af1)