• 
      

    Refactor logic out of UploadDiffForm

    Review Request #4231 — Created June 11, 2013 and submitted

    Information

    Review Board
    master

    Reviewers

    Refactor logic out of UploadDiffForm
    
    All of the logic for taking a diff file and creating a DiffSet was in
    UploadDiffForm. This change moves it out into a manager, so that I'll be able to
    use the same code for creating DiffSets from diff data fetched from an upstream
    repository.
    
    Ran unit tests.
    Description From Last Updated

    Same line.

    chipx86 chipx86

    Col: 20 E221 multiple spaces before operator

    reviewbot reviewbot

    It wasn't immediately clear to me how this is "form data," except for where it was called from. I eventually …

    chipx86 chipx86

    Col: 9 E303 too many blank lines (2)

    reviewbot reviewbot

    Col: 20 E127 continuation line over-indented for visual indent

    reviewbot reviewbot

    Col: 22 E127 continuation line over-indented for visual indent

    reviewbot reviewbot

    Could simplify by just passing get_file_exists, and just have that function take self. Same with the others.

    chipx86 chipx86

    Col: 80 E501 line too long (82 > 79 characters)

    reviewbot reviewbot

    Col: 38 E127 continuation line over-indented for visual indent

    reviewbot reviewbot

    I think they're UploadedFile objects, not FileFields, aren't they?

    chipx86 chipx86
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/diffviewer/forms.py
          reviewboard/diffviewer/models.py
          reviewboard/diffviewer/managers.py
          reviewboard/diffviewer/tests.py
        Ignored Files:
      
      
    2. reviewboard/diffviewer/managers.py (Diff revision 1)
       
       
      Show all issues
      Col: 20
       E221 multiple spaces before operator
      
    3. reviewboard/diffviewer/managers.py (Diff revision 1)
       
       
      Show all issues
      Col: 9
       E303 too many blank lines (2)
      
    4. reviewboard/diffviewer/managers.py (Diff revision 1)
       
       
      Show all issues
      Col: 20
       E127 continuation line over-indented for visual indent
      
    5. reviewboard/diffviewer/managers.py (Diff revision 1)
       
       
      Show all issues
      Col: 22
       E127 continuation line over-indented for visual indent
      
    6. reviewboard/diffviewer/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (82 > 79 characters)
      
    7. reviewboard/diffviewer/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 38
       E127 continuation line over-indented for visual indent
      
    8. 
        
    chipx86
    1. 
        
    2. reviewboard/diffviewer/managers.py (Diff revision 1)
       
       
       
      Show all issues
      Same line.
    3. reviewboard/diffviewer/managers.py (Diff revision 1)
       
       
      Show all issues
      It wasn't immediately clear to me how this is "form data," except for where it was called from. I eventually realized the diff_file and parent_diff_file are UploadedFile objects from a form. Maybe rename this to create_from_upload?
      
      Also, can you add docstrings for this and create_from_data? It'll help make it clear how they differ.
      1. What were your thoughts on the name change?
      2. OK.
    4. reviewboard/diffviewer/tests.py (Diff revision 1)
       
       
       
      Show all issues
      Could simplify by just passing get_file_exists, and just have that function take self. Same with the others.
    5. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/diffviewer/forms.py
          reviewboard/diffviewer/models.py
          reviewboard/diffviewer/managers.py
          reviewboard/diffviewer/tests.py
        Ignored Files:
      
      
    2. 
        
    chipx86
    1. 
        
    2. reviewboard/diffviewer/managers.py (Diff revision 2)
       
       
      Show all issues
      I think they're UploadedFile objects, not FileFields, aren't they?
    3. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/diffviewer/forms.py
          reviewboard/diffviewer/models.py
          reviewboard/diffviewer/managers.py
          reviewboard/diffviewer/tests.py
        Ignored Files:
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/webapi/resources.py
          reviewboard/diffviewer/models.py
          reviewboard/diffviewer/errors.py
          reviewboard/diffviewer/tests.py
          reviewboard/reviews/forms.py
          reviewboard/diffviewer/forms.py
          reviewboard/diffviewer/managers.py
        Ignored Files:
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (2d8174d).