Summary

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

Review Request #7265 — Created April 29, 2015 and submitted May 6, 2015, 5:43 p.m.

Information

Owner

smacleod

Repository

Review Board

Branch

release-2.0.x

Bugs

Depends On

~~7264~~

Commit

f2fc51f...

Reviewers

Groups

reviewboard

People

Description

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()

Testing Done

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

Issues

Description	From	Last Updated
're' imported but unused	reviewbot	May 4, 2015, 9:55 a.m.
Single quotes.	brennie	May 4, 2015, 9:56 a.m.
Single quotes.	brennie	May 4, 2015, 9:56 a.m.
'FileNotFoundError' imported but unused	reviewbot	May 4, 2015, 9:55 a.m.
No need for parens here.	chipx86	May 4, 2015, 9:57 a.m.
local variable 'new_changeset_id' is assigned to but never used	reviewbot	May 4, 2015, 5:49 p.m.
local variable 'orig_changeset_id' is assigned to but never used	reviewbot	May 4, 2015, 5:49 p.m.
Col: 17 E221 multiple spaces before operator	reviewbot	May 4, 2015, 5:51 p.m.
Col: 21 E221 multiple spaces before operator	reviewbot	May 4, 2015, 5:51 p.m.
We could probably make this a utility function, cutting down this code a lot and making it more reusable. Another …	chipx86	May 4, 2015, 7:28 p.m.
Col: 17 E221 multiple spaces before operator	reviewbot	May 5, 2015, 8:44 a.m.
Col: 17 E221 multiple spaces before operator	reviewbot	May 5, 2015, 8:44 a.m.
'warnings' imported but unused	reviewbot	May 5, 2015, 8:54 a.m.

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

reviewboard/scmtools/hg.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 're' imported but unused
```
reviewboard/scmtools/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'FileNotFoundError' imported but unused
```

Ship it!

```
Looks good to me!
```
reviewboard/scmtools/hg.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Single quotes.
```
reviewboard/scmtools/hg.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Single quotes.
```

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.

reviewboard/scmtools/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for parens here.
```

Change Summary:

Addressed feedback - tests, file_exists fixed.

Description:

		All SCMTool implementations should now take keyword arguments and a
~		`base_commit_id` argument. All internal implementations have been
	~	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
	~	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
	~	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()

Testing Done:

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

Commit:

d2607206557ad40a91a0e605fdbe4daf82abb690

280a3b216ca33376b1fa954c5df6e1eb190db1ac

Diff:

Revision 2 (+148 -40)

Show changes

	reviewboard/scmtools/bzr.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/cvs.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/localfile.py
	reviewboard/scmtools/models.py
	6 more

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

reviewboard/scmtools/hg.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 local variable 'new_changeset_id' is assigned to but never used
```
reviewboard/scmtools/hg.py (Diff revision 2)
The issue has been resolved. Show all issues
```
 local variable 'orig_changeset_id' is assigned to but never used
```

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

reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 17
 E221 multiple spaces before operator
```
reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 21
 E221 multiple spaces before operator
```

Change Summary:

Review Bot fixups.

Commit:

280a3b216ca33376b1fa954c5df6e1eb190db1ac

85e3cd5394d78b0ceae270c0201e04eca73dca53

Diff:

Revision 3 (+148 -40)

Show changes

	reviewboard/scmtools/bzr.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/cvs.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/localfile.py
	reviewboard/scmtools/models.py
	6 more

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

reviewboard/scmtools/tests.py (Diff revision 3)

The issue has been resolved. 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.

chipx86 May 4, 2015, 6:28 p.m.

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.

smacleod May 4, 2015, 7:02 p.m.

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?

chipx86 May 4, 2015, 7:06 p.m.

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.

Change Summary:

Fiuxed issue.

Commit:

85e3cd5394d78b0ceae270c0201e04eca73dca53

816a14e981e7ff2ed17c442a2f999745f10f32ad

Diff:

Revision 4 (+163 -40)

Show changes

	reviewboard/scmtools/bzr.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/cvs.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/localfile.py
	reviewboard/scmtools/models.py
	7 more

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

reviewboard/scmtools/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 17
 E221 multiple spaces before operator
```
reviewboard/scmtools/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 17
 E221 multiple spaces before operator
```

Ship it!

```
Ship It!
```

Change Summary:

Fixed PEP8. Apparently I like to hit the spacebar too much :P

Commit:

816a14e981e7ff2ed17c442a2f999745f10f32ad

c715ebb10a78512be1d93ee2deb910ecd2b6e56e

Diff:

Revision 5 (+163 -40)

Show changes

	reviewboard/scmtools/bzr.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/cvs.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/localfile.py
	reviewboard/scmtools/models.py
	7 more

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

reviewboard/scmtools/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
 'warnings' imported but unused
```

Commit:

c715ebb10a78512be1d93ee2deb910ecd2b6e56e

f2fc51f48d53204f4e755f17dd59330dfef01302

Diff:

Revision 6 (+162 -40)

Show changes

	reviewboard/scmtools/bzr.py
	reviewboard/scmtools/clearcase.py
	reviewboard/scmtools/core.py
	reviewboard/scmtools/cvs.py
	reviewboard/scmtools/git.py
	reviewboard/scmtools/hg.py
	reviewboard/scmtools/localfile.py
	reviewboard/scmtools/models.py
	7 more

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

Status:: Completed
Change Summary:: Pushed to release-2.0.x (6b34af1)