Expand diff parsing for additional metadata and multiple changes.

 






 

 


  Review Request #11744
  —
  Created July 26, 2021 and submitted 

 




















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  release-4.0.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Historically, our diff parsing infrastructure was built around a base

set of manual line-by-line parsing rules, which could be specialized.

The result would be a list of files. This was expanded a bit in 4.0 and

subsequent releases to collect and handle additional data (such as being

able to populate FileDiff.extra_data), but the core design remained.


With DiffX coming, we need more. This change expands our diff parsing

support.


New ParsedDiff and ParsedDiffChange classes were added, which

represent information on a diff as a whole, and a change within it,

respectively. These both provide support for populating extra_data,

which will be set in DiffSet and DiffCommit when parsing a diff.


A new BaseDiffParser has been introduced as the new base class, which

DiffParser subclasses. The new base class does very little, and is

more of an interface. It contains no built-in parsing logic. This will

allow for more specialized parsers with their own logic.


BaseDiffParser and DiffParser now provide a parse_diff() method,

which is the new method to call to parse a diff. This replaces

parse(), which still exists in DiffParser.


For new subclasses of BaseDiffParser, a parse_diff() implementation

will be responsible for parsing into a series of ParsedDiff,

ParsedDiffChange, and ParsedDiffFile objects.


For subclasses of DiffParser, parse() can still be overridden. The

new default DiffParser.parse_diff() will set up the new objects as

parsed_diff and parsed_diff_change attributes.


This new model will make it easier to implement DiffX, and will give us

some good capabilities down the road. For the moment, there are still

limitations based on our existing process of uploading and parsing

diffs. The main one being that, while a ParsedDiff (and later a DiffX)

can in theory have multiple changes, it's not actually allowed through

our parsing. Clients of the API will need to provide diffs with only a

single change, until we're able to implement the appropriate changes

in the diff upload API and related code (probably post-4.0 sometime).

 








  


 
 

  
Unit tests pass on Python 2 and 3.


Tested posting diffs for review, without any issues.

 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Expand diff parsing for additional metadata and multiple changes.
      
Historically, our diff parsing infrastructure was built around a base
set of manual line-by-line parsing rules, which could be specialized.
The result would be a list of files. This was expanded a bit in 4.0 and
subsequent releases to collect and handle additional data (such as being
able to populate `FileDiff.extra_data`), but the core design remained.

With DiffX coming, we need more. This change expands our diff parsing
support.

New `ParsedDiff` and `ParsedDiffChange` classes were added, which
represent information on a diff as a whole, and a change within it,
respectively. These both provide support for populating `extra_data`,
which will be set in `DiffSet` and `DiffCommit` when parsing a diff.

A new `BaseDiffParser` has been introduced as the new base class, which
`DiffParser` subclasses. The new base class does very little, and is
more of an interface. It contains no built-in parsing logic. This will
allow for more specialized parsers with their own logic.

`BaseDiffParser` and `DiffParser` now provide a `parse_diff()` method,
which is the new method to call to parse a diff. This replaces
`parse()`, which still exists in `DiffParser`.

For new subclasses of `BaseDiffParser`, a `parse_diff()` implementation
will be responsible for parsing into a series of `ParsedDiff`,
`ParsedDiffChange`, and `ParsedDiffFile` objects.

For subclasses of `DiffParser`, `parse()` can still be overridden. The
new default `DiffParser.parse_diff()` will set up the new objects as
`parsed_diff` and `parsed_diff_change` attributes.

This new model will make it easier to implement DiffX, and will give us
some good capabilities down the road. For the moment, there are still
limitations based on our existing process of uploading and parsing
diffs. The main one being that, while a `ParsedDiff` (and later a DiffX)
can in theory have multiple changes, it's not actually allowed through
our parsing. Clients of the API will need to provide diffs with only a
single change, until we're able to implement the appropriate changes
in the diff upload API and related code (probably post-4.0 sometime).

     


    		
    d5e07d83714ec06c81be11a876cdb4914f3412f1

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
F821 undefined name 'parser'

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F821 undefined name 'parsed_diff_file'

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F401 'reviewboard.scmtools.core.Revision' imported but unused

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
F821 undefined name 'parent_diff_change'

      		
      
       reviewbotreviewbot
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
Change Summary:

 
    

  • Removed a duplicate assertion.
    • 

  • Fixed a variable access.
    • 

  • Removed an unused import.
    • 






 



  

   
Commits:

   
    





 

  
   


    

    Summary
    ID

   

  
  

   


    

    

     

      Expand diff parsing for additional metadata and multiple changes.
      
Historically, our diff parsing infrastructure was built around a base
set of manual line-by-line parsing rules, which could be specialized.
The result would be a list of files. This was expanded a bit in 4.0 and
subsequent releases to collect and handle additional data (such as being
able to populate `FileDiff.extra_data`), but the core design remained.

With DiffX coming, we need more. This change expands our diff parsing
support.

New `ParsedDiff` and `ParsedDiffChange` classes were added, which
represent information on a diff as a whole, and a change within it,
respectively. These both provide support for populating `extra_data`,
which will be set in `DiffSet` and `DiffCommit` when parsing a diff.

A new `BaseDiffParser` has been introduced as the new base class, which
`DiffParser` subclasses. The new base class does very little, and is
more of an interface. It contains no built-in parsing logic. This will
allow for more specialized parsers with their own logic.

`BaseDiffParser` and `DiffParser` now provide a `parse_diff()` method,
which is the new method to call to parse a diff. This replaces
`parse()`, which still exists in `DiffParser`.

For new subclasses of `BaseDiffParser`, a `parse_diff()` implementation
will be responsible for parsing into a series of `ParsedDiff`,
`ParsedDiffChange`, and `ParsedDiffFile` objects.

For subclasses of `DiffParser`, `parse()` can still be overridden. The
new default `DiffParser.parse_diff()` will set up the new objects as
`parsed_diff` and `parsed_diff_change` attributes.

This new model will make it easier to implement DiffX, and will give us
some good capabilities down the road. For the moment, there are still
limitations based on our existing process of uploading and parsing
diffs. The main one being that, while a `ParsedDiff` (and later a DiffX)
can in theory have multiple changes, it's not actually allowed through
our parsing. Clients of the API will need to provide diffs with only a
single change, until we're able to implement the appropriate changes
in the diff upload API and related code (probably post-4.0 sometime).

     


    		
    049886e6eb19d624326f59a7432adf5aa8520b97

   




   


    

    

     

      Expand diff parsing for additional metadata and multiple changes.
      
Historically, our diff parsing infrastructure was built around a base
set of manual line-by-line parsing rules, which could be specialized.
The result would be a list of files. This was expanded a bit in 4.0 and
subsequent releases to collect and handle additional data (such as being
able to populate `FileDiff.extra_data`), but the core design remained.

With DiffX coming, we need more. This change expands our diff parsing
support.

New `ParsedDiff` and `ParsedDiffChange` classes were added, which
represent information on a diff as a whole, and a change within it,
respectively. These both provide support for populating `extra_data`,
which will be set in `DiffSet` and `DiffCommit` when parsing a diff.

A new `BaseDiffParser` has been introduced as the new base class, which
`DiffParser` subclasses. The new base class does very little, and is
more of an interface. It contains no built-in parsing logic. This will
allow for more specialized parsers with their own logic.

`BaseDiffParser` and `DiffParser` now provide a `parse_diff()` method,
which is the new method to call to parse a diff. This replaces
`parse()`, which still exists in `DiffParser`.

For new subclasses of `BaseDiffParser`, a `parse_diff()` implementation
will be responsible for parsing into a series of `ParsedDiff`,
`ParsedDiffChange`, and `ParsedDiffFile` objects.

For subclasses of `DiffParser`, `parse()` can still be overridden. The
new default `DiffParser.parse_diff()` will set up the new objects as
`parsed_diff` and `parsed_diff_change` attributes.

This new model will make it easier to implement DiffX, and will give us
some good capabilities down the road. For the moment, there are still
limitations based on our existing process of uploading and parsing
diffs. The main one being that, while a `ParsedDiff` (and later a DiffX)
can in theory have multiple changes, it's not actually allowed through
our parsing. Clients of the API will need to provide diffs with only a
single change, until we're able to implement the appropriate changes
in the diff upload API and related code (probably post-4.0 sometime).

     


    		
    d25cf267a2964095fa3bde32795b19c1eba1cf26

   



  
 







   		
  




  

   
Diff:

   
    
 Revision 2 (+1164 -156)
Show changes
 
    reviewboard/diffviewer/filediff_creator.py 
    reviewboard/diffviewer/parser.py 
    reviewboard/diffviewer/tests/test_diff_parser.py 
    reviewboard/diffviewer/tests/test_diffcommit_manager.py 
    reviewboard/diffviewer/tests/test_diffset_manager.py 
    reviewboard/scmtools/core.py 
 

   		
  



 









 

  
Checks run (1 failed, 1 succeeded)



  

 flake8
 failed.
 





  

 JSHint
 passed.
 





 







 
flake8

 


 















  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-4.0.x (e2b6dcc)





 


 










  

 









     

    

   

  

  

   
   Loading...