Summary

Add support for -X/--exclude/EXCLUDE_FILES.

Review Request #6317 — Created Sept. 12, 2014 and submitted Sept. 17, 2014, 12:28 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

master

Bugs

Depends On

Blocks

6331

Commit

422122c...

Reviewers

Groups

rbtools, students

People

Description

rbtools now recognizes the -X and --exclude command line options for
excluding files from diffs and review requests. The EXCLUDE_FILES
option in .reviewboardrc adds the same functionality, but can be
committed to the repository.

Currently no backends support this option.

Testing Done

Unit tests pass.
Running rbt diff with -X or --exclude flags, or with
EXCLUDE_FILES set in .reviewboardrc causes the expected error
message

Issues

Description	From	Last Updated
The SCMClients shouldn't be handling these as command line arguments. Instead, diff should be updated to take an excluded_files= argument …	chipx86	Sept. 14, 2014, 10:26 p.m.
"or"	chipx86	Sept. 14, 2014, 10:26 p.m.
Can we rename the existing parameter files to be include_files, and put the new parameter right after that one? Same …	david	Sept. 15, 2014, 10:08 p.m.
We prefer the % formatting scheme over this one (change "{0}" to "%s" and replace ".format(tool.name)" with "% tool.name").	david	Sept. 15, 2014, 10:08 p.m.
Can you wrap the whole conditional in parens and remove the continuation character?	david	Sept. 15, 2014, 10:08 p.m.
Same comment about .format vs %	david	Sept. 15, 2014, 10:08 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 15, 2014, 10:10 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/__init__.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/__init__.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst

Commit:

c538907b8a27e815d342aee0f878aee339328592

10202e0005e98b1d17e04042764acb5f1c1fad16

Diff:

Revision 2 (+81 -1)

Show changes

	AUTHORS
	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	4 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/__init__.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/__init__.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS

rbtools/clients/clearcase.py (Diff revision 2)

The issue has been resolved. Show all issues

The SCMClients shouldn't be handling these as command line arguments. Instead, diff should be updated to take an excluded_files= argument and use that insead. We're trying to eventually move away from having self.options.

I'm also hoping we can move away from these errors within diff. Instead, how about adding a supports_diff_exclude_files flag on the class, and having the caller check this and provide the error? (We should probably do this for include as well, but that'd be a separate change.)

rbtools/clients/cvs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"or"
```

Change Summary:

Add an exclude_files parameter to diff methods in all SCMClients.

Error handling for whether or not a tool can handle excluding is done
in the diff and post commands via checking the
SCMTool.supports_diff_exclude_files flag.

Testing Done:

	+	Unit tests pass.
	+	Running `rbt diff` with `-X` or `--exclude` flags, or with
	+	`EXCLUDE_FILES` set in `.reviewboardrc` causes the expected error
	+	message

Commit:

10202e0005e98b1d17e04042764acb5f1c1fad16

4a3c7be0aeb917f6dfe822f7d257bf718365202f

Diff:

Revision 3 (+51 -9)

Show changes

	AUTHORS
	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS

rbtools/clients/__init__.py (Diff revision 3)

The issue has been resolved. Show all issues

Can we rename the existing parameter files to be include_files, and put the new parameter right after that one? Same in the rest of the clients.

rbtools/commands/diff.py (Diff revision 3)

The issue has been resolved. Show all issues

We prefer the % formatting scheme over this one (change "{0}" to "%s" and replace ".format(tool.name)" with "% tool.name").

rbtools/commands/post.py (Diff revision 3)

The issue has been resolved. Show all issues

Can you wrap the whole conditional in parens and remove the continuation character?

rbtools/commands/post.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same comment about .format vs %
```

Change Summary:

Rename files -> include_files in SCMClient.diff methods.

Use % formatting instead of str.format()

Fix formatting in multi-line if statement.

Commit:

4a3c7be0aeb917f6dfe822f7d257bf718365202f

e60ebd0c57199b5a122ff0e7748573fac092ec12

Diff:

Revision 4 (+78 -26)

Show changes

	AUTHORS
	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS

rbtools/clients/cvs.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fix line too long error.

Commit:

e60ebd0c57199b5a122ff0e7748573fac092ec12

422122cedafee46dbd68733ccb1ab4952d15cbe0

Diff:

Revision 5 (+78 -26)

Show changes

	AUTHORS
	docs/rbtools/rbt/commands/diff.rst
	docs/rbtools/rbt/commands/post.rst
	rbtools/clients/__init__.py
	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	7 more

Tool: Pyflakes
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/cvs.py
    rbtools/commands/post.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/clients/clearcase.py
    rbtools/clients/plastic.py
    rbtools/clients/__init__.py
    rbtools/clients/mercurial.py
    rbtools/clients/git.py
    rbtools/clients/perforce.py
    rbtools/clients/bazaar.py
    rbtools/clients/svn.py

Ignored Files:
    docs/rbtools/rbt/commands/post.rst
    docs/rbtools/rbt/commands/diff.rst
    AUTHORS

Ship it!

This looks good to me, but I'd like to hold off on pushing it until there's an implementation for at least one SCMClient. You can build on this by creating a new branch on top of this and then posting changes with rbt post <thisbranch>..<newbranch>

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-0.6.x (7091573)