Add start index in base differ class and update MyersDiff accordingly.

Review Request #11253 — Created Oct. 28, 2020 and updated

bnie
Review Board
release-4.0.x
reviewboard, students

The current differ assumes the line index always starts at zero and the
get_opcodes() function in MyersDiffer calculates the line number base
on this assumption.

However, one future differ option, PatienceDiffer, which uses
MyersDiffer to obtain the final diff, passes in portions of the overall
file seperately and the start index will not be line 0 for code chunks
that do not start at the head of the file. As a result, some index
varables needs to be set in the differ to keep track of which line
each code block starts at.

This commit adds such two new parameters, a_start_index and
b_start_index in the base differ class and updates MyersDiffer to
generate correct opcode after the new change.

Four new test cases are added to make sure MyersDiffer works, given
different start indices for file a and b, for all the opertaions: equal,
replace, insert and delete. Also, another test case with same non-zero
start indices is added to make sure MyersDiffer works when the code
chunk is at the same location but not the start of the file.

Summary
Added two more parameter to indicate start index of a and b file, and modified
Addressed comment from Christian.
Description From Last Updated

The review request should be limited to 79 characters per line.

keanwengkeanweng

You might also want to add the Students group to the Reviewers.

keanwengkeanweng

I'm not too familiar with the MyersDiff so I can't really comment on the code. However, I downloaded the patch ...

keanwengkeanweng

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

For readability, let's use keyword arguments and begin the arguments on the next line (4 spaces indented). Same below.

chipx86chipx86

Can you add ad ocstring here? I know the other function doesn't have it, but it predates our modern doc ...

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

bnie
keanweng
  1. 
      
  2. The review request should be limited to 79 characters per line.

  3. You might also want to add the Students group to the Reviewers.

  4. 
      
jblazusi
  1. Everything looks great aside from the things mentioned by Kean.

  2. 
      
bnie
chipx86
  1. This looks great. Small stylistic stuff, and then let's land this! :)

  2. reviewboard/diffviewer/tests/test_myersdiff.py (Diff revision 2)
     
     
     
     
     

    For readability, let's use keyword arguments and begin the arguments on the next line (4 spaces indented).

    Same below.

  3. Can you add ad ocstring here? I know the other function doesn't have it, but it predates our modern doc standards.

  4. 
      
bnie
Review request changed

Change Summary:

Added doc string to test function and used keyword argument in all test cases.

Commits:

Summary
-
Added two more parameter to indicate start index of a and b file, and modified
+
Added two more parameter to indicate start index of a and b file, and modified
+
Addressed comment from Christian.

Diff:

Revision 3 (+245 -73)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
keanweng
  1. 
      
  2. I'm not too familiar with the MyersDiff so I can't really comment on the code.
    However, I downloaded the patch and ran your unit tests. All 9 of them passed, great work!

  3. 
      
Loading...