Summary

Fix output of rbt diff/post for git when -X specified.

Review Request #6350 — Created Sept. 21, 2014 and submitted Sept. 21, 2014, 3:23 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

release-0.6.x

Bugs

Depends On

Commit

3492099...

Reviewers

Groups

rbtools, students

People

Description

Currently, git's filter support does the opposite behaviour
(only includes files that match all exclude_patterns).

A new module, rbtools.utils.diffs, specifies helper functions
filename_match_any_patterns, which determines if a filename matches
at least one of the given patterns, and
remove_filenames_matching_patterns, which returns an iterable of the
filenames that do not match any given pattern.

The parameters to git diff and git diff-tree have been
reordered so that the revision specification is always the last
argument before the filenames.

Testing Done

All unit tests pass now.
Generated diffs with rbt diff in a git repository with more than
one file changed. When -X filename was specified, only that
file's diff was excluded from the output.

Issues

Description	From	Last Updated
'functools' imported but unused	reviewbot	Sept. 21, 2014, 1:39 p.m.
'itertools' imported but unused	reviewbot	Sept. 21, 2014, 1:39 p.m.
'fnmatch' imported but unused	reviewbot	Sept. 21, 2014, 1:39 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	Sept. 21, 2014, 1:39 p.m.
Imports should be in alphabetical order	AS asalahli	Sept. 21, 2014, 2:07 p.m.
Blank line between these.	chipx86	Sept. 21, 2014, 2:13 p.m.
Not sure if this matters, but concatenating two lists is more computationally expensive than list creation. Feel free to drop …	AS asalahli	Sept. 21, 2014, 2:06 p.m.
Would you mind documenting the output we expect to get briefly, so that it's obvious why we're doing filename.split()[-1]?	chipx86	Sept. 21, 2014, 2:13 p.m.
I feel that the function names are a bit generic, given that we're specifically working with filenames. How about filename_matches_any_pattern?	chipx86	Sept. 21, 2014, 2:14 p.m.
Same idea here.	chipx86	Sept. 21, 2014, 2:21 p.m.
Small nit, but when dealing with multi-line generators or list comprehensions, we usually do: ( filename for ... ... )	chipx86	Sept. 21, 2014, 2:14 p.m.
Nit - for every new .py file, we like to include: from future import unicode_literals at the top of the …	mike_conley	Sept. 21, 2014, 2:56 p.m.
The closing parenthesis should probably not be in a seperate line	AS asalahli	Sept. 21, 2014, 2:31 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/utils/diffs.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/diffs.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'functools' imported but unused
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'itertools' imported but unused
```
rbtools/clients/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'fnmatch' imported but unused
```
rbtools/utils/diffs.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

Change Summary:

Add students group.

Commit:

766e24e72bdf02d944ae0acc97cb95b4b271778d

c73434dc32afbfe5c370beaf9028700bd55c58b6

Groups:

students

Diff:

Revision 2 (+31 -19)

Show changes

	rbtools/clients/git.py
	rbtools/utils/diffs.py

Tool: Pyflakes
Processed Files:
    rbtools/utils/diffs.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/diffs.py
    rbtools/clients/git.py

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Imports should be in alphabetical order
```

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

The issue has been resolved. Show all issues

Not sure if this matters, but concatenating two lists is more computationally expensive than list creation. Feel free to drop this issue :)

rbtools/clients/git.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

The issue has been resolved. Show all issues

Would you mind documenting the output we expect to get briefly, so that it's obvious why we're doing filename.split()[-1]?

rbtools/utils/diffs.py (Diff revision 2)

The issue has been resolved. Show all issues

I feel that the function names are a bit generic, given that we're specifically working with filenames. How about filename_matches_any_pattern?

rbtools/utils/diffs.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same idea here.
```

rbtools/utils/diffs.py (Diff revision 2)

The issue has been resolved. Show all issues

Small nit, but when dealing with multi-line generators or list comprehensions, we usually do:

(
    filename
    for ...
    ...
)

Change Summary:

Explain the output of git diff-tree

Style changes.

Better names for functions.

Commit:

c73434dc32afbfe5c370beaf9028700bd55c58b6

3492099029e898224cc96bcf0163faa73760b7f1

Diff:

Revision 3 (+35 -18)

Show changes

	rbtools/clients/git.py
	rbtools/utils/diffs.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/utils/diffs.py
    rbtools/clients/git.py



Tool: Pyflakes
Processed Files:
    rbtools/utils/diffs.py
    rbtools/clients/git.py

rbtools/utils/diffs.py (Diff revision 3)
The issue has been dropped. Show all issues
```
The closing parenthesis should probably not be in a seperate line
```
1. brennie Sept. 21, 2014, 2:32 p.m.
  See review by Christian.
2. AS
  
  asalahli Sept. 21, 2014, 2:33 p.m.
  Oh, ok

Ship it!

```
Ship It!
```

Ship it!

This looks good to me, but you might want an additional ship-it from one of the other mentors, since I'm not 100% up on the RBTools stuff.

rbtools/utils/diffs.py (Diff revision 3)

The issue has been dropped. Show all issues

Nit - for every new .py file, we like to include:
from future import unicode_literals
at the top of the file.

chipx86 Sept. 21, 2014, 2:52 p.m.

This only applies to the RB tree. RBTools will break on Python 2.4 with this.

chipx86 Sept. 21, 2014, 2:52 p.m.
```
(And 2.5)
```
mike_conley Sept. 21, 2014, 3:04 p.m.
```
Whoops - good to know. Thanks!
```

Change Summary:: Update names of functions in rr description.
Description:: Currently, git's filter support does the opposite behaviour
(only includes files that match all exclude_patterns).

A new module, rbtools.utils.diffs, specifies helper functions
~ match_any_patterns, which determines if a filename matches at
~ least one of the given patterns, and remove_pattern_matches,
~ which returns an iterable of the filenames that do not match any
~ given pattern.
~ filename_match_any_patterns, which determines if a filename matches
~ at least one of the given patterns, and
~ remove_filenames_matching_patterns, which returns an iterable of the
~ filenames that do not match any given pattern.

The parameters to git diff and git diff-tree have been
reordered so that the revision specification is always the last
argument before the filenames.

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

		Currently, git's filter support does the opposite behaviour
		(only includes files that match all exclude_patterns).

		A new module, `rbtools.utils.diffs`, specifies helper functions
~		`match_any_patterns`, which determines if a filename matches at
~		least one of the given patterns, and `remove_pattern_matches`,
~		which returns an iterable of the filenames that do not match any
~		given pattern.
	~	`filename_match_any_patterns`, which determines if a filename matches
	~	at least one of the given patterns, and
	~	`remove_filenames_matching_patterns`, which returns an iterable of the
	~	filenames that do not match any given pattern.

		The parameters to `git diff` and `git diff-tree` have been
		reordered so that the revision specification is always the last
		argument before the filenames.