Fix output of rbt diff/post for git when -X specified.
Review Request #6350 — Created Sept. 21, 2014 and submitted
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 diffandgit diff-treehave been
reordered so that the revision specification is always the last
argument before the filenames.
All unit tests pass now.
Generated diffs withrbt diffin a git repository with more than
one file changed. When-X filenamewas specified, only that
file's diff was excluded from the output.
| Description | From | Last Updated |
|---|---|---|
|
'functools' imported but unused |
 reviewbot
|
|
|
'itertools' imported but unused |
reviewbot
|
|
|
'fnmatch' imported but unused |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Imports should be in alphabetical order |
AS asalahli | |
|
Blank line between these. |
 chipx86
|
|
|
Not sure if this matters, but concatenating two lists is more computationally expensive than list creation. Feel free to drop … |
AS asalahli | |
|
Would you mind documenting the output we expect to get briefly, so that it's obvious why we're doing filename.split()[-1]? |
chipx86
|
|
|
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
|
|
|
Same idea here. |
chipx86
|
|
|
Small nit, but when dealing with multi-line generators or list comprehensions, we usually do: ( filename for ... ... ) |
chipx86
|
|
|
Nit - for every new .py file, we like to include: from future import unicode_literals at the top of the … |
 mike_conley
|
|
|
The closing parenthesis should probably not be in a seperate line |
AS asalahli |
Change Summary:
Add students group.
Commit: |
|
||||
|---|---|---|---|---|---|
Groups: |
|
||||
Diff: |
Revision 2 (+31 -19) |
-
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
AS
-
-
rbtools/clients/git.py (Diff revision 2) 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) 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) 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) 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: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+35 -18) |
-
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
AS
-
-
rbtools/utils/diffs.py (Diff revision 3) The closing parenthesis should probably not be in a seperate line
-
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) Nit - for every new .py file, we like to include:
from future import unicode_literals
at the top of the file.
Change Summary:
Update names of functions in rr description.
Description: |
|
|---|
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