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

Review Request #6350 — Created Sept. 21, 2014 and submitted

Information

RBTools
release-0.6.x
3492099...

Reviewers

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.

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.

Description From Last Updated

'functools' imported but unused

reviewbotreviewbot

'itertools' imported but unused

reviewbotreviewbot

'fnmatch' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Imports should be in alphabetical order

AS asalahli

Blank line between these.

chipx86chipx86

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]?

chipx86chipx86

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

chipx86chipx86

Same idea here.

chipx86chipx86

Small nit, but when dealing with multi-line generators or list comprehensions, we usually do: ( filename for ... ... )

chipx86chipx86

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

mike_conleymike_conley

The closing parenthesis should probably not be in a seperate line

AS asalahli
reviewbot
  1. 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
    
    
  2. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
     'functools' imported but unused
    
  3. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
     'itertools' imported but unused
    
  4. rbtools/clients/git.py (Diff revision 1)
     
     
    Show all issues
     'fnmatch' imported but unused
    
  5. rbtools/utils/diffs.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  6. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
AS
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     
    Show all issues

    Imports should be in alphabetical order

  3. 
      
AS
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     
    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 :)

  3. 
      
chipx86
  1. 
      
  2. rbtools/clients/git.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

  3. rbtools/clients/git.py (Diff revision 2)
     
     
    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]?

  4. rbtools/utils/diffs.py (Diff revision 2)
     
     
    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?

  5. rbtools/utils/diffs.py (Diff revision 2)
     
     
    Show all issues

    Same idea here.

  6. rbtools/utils/diffs.py (Diff revision 2)
     
     
     
     
    Show all issues

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

    (
        filename
        for ...
        ...
    )
    
  7. 
      
brennie
reviewbot
  1. 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
    
    
  2. 
      
AS
  1. 
      
  2. rbtools/utils/diffs.py (Diff revision 3)
     
     
    Show all issues

    The closing parenthesis should probably not be in a seperate line

  3. 
      
AS
  1. Ship It!

  2. 
      
mike_conley
  1. 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.

  2. rbtools/utils/diffs.py (Diff revision 3)
     
     
    Show all issues

    Nit - for every new .py file, we like to include:

    from future import unicode_literals

    at the top of the file.

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

    2. (And 2.5)

    3. Whoops - good to know. Thanks!

  3. 
      
brennie
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.6.x (cbbb365)