Summary

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

Review Request #11253 — Created Oct. 28, 2020 and updated Dec. 4, 2020, 1:09 a.m.

Information

Owner

bnie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Commits

Summary	ID
Added two more parameter to indicate start index of a and b file, and modified unit tests accordingly.	39038ac9d7385bbaf916b4cd11f68ad00dd8268f
Addressed comment from Christian.	678fb08f92a145a7f6baf65b7edbdc53d10bc784

Issues

Description	From	Last Updated
The review request should be limited to 79 characters per line.	keanweng	Nov. 27, 2020, 12:57 a.m.
You might also want to add the Students group to the Reviewers.	keanweng	Nov. 27, 2020, 12:57 a.m.
I'm not too familiar with the MyersDiff so I can't really comment on the code. However, I downloaded the patch …	keanweng	Dec. 4, 2020, 1:09 a.m.
E128 continuation line under-indented for visual indent	reviewbot	Oct. 28, 2020, 9:23 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 28, 2020, 9:23 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 28, 2020, 9:23 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 28, 2020, 9:23 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 28, 2020, 9:23 p.m.
For readability, let's use keyword arguments and begin the arguments on the next line (4 spaces indented). Same below.	chipx86	Dec. 2, 2020, 5:31 p.m.
Can you add ad ocstring here? I know the other function doesn't have it, but it predates our modern doc …	chipx86	Dec. 2, 2020, 5:31 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_myersdiff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/diffviewer/tests/test_myersdiff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_myersdiff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_myersdiff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
reviewboard/diffviewer/tests/test_myersdiff.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```

Commits:

	Summary	ID
	Added two more parameter to indicate start index of a and b file, and modified unit tests accordingly.	424109a22d204b6c579dc178a9bffc1ab1610a08
	Added two more parameter to indicate start index of a and b file, and modified unit tests accordingly.	3995352b38e41105fce1d514d7088a649b1e0a37

Diff:

Revision 2 (+140 -14)

Show changes

	reviewboard/diffviewer/differ.py
	reviewboard/diffviewer/myersdiff.py
	reviewboard/diffviewer/tests/test_myersdiff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

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

The issue has been resolved. Show all issues

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

Everything looks great aside from the things mentioned by Kean.

Description:: ~
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.
~
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.
~
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.
~
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.
Testing Done:: ~
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.
~
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.
Groups:: students

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

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

The issue has been resolved. Show all issues

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

Same below.

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

The issue has been resolved. Show all issues

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

Change Summary:

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

Commits:

	Summary	ID
	Added two more parameter to indicate start index of a and b file, and modified unit tests accordingly.	3995352b38e41105fce1d514d7088a649b1e0a37
	Added two more parameter to indicate start index of a and b file, and modified unit tests accordingly.	39038ac9d7385bbaf916b4cd11f68ad00dd8268f
	Addressed comment from Christian.	678fb08f92a145a7f6baf65b7edbdc53d10bc784

Diff:

Revision 3 (+245 -73)

Show changes

	reviewboard/diffviewer/differ.py
	reviewboard/diffviewer/myersdiff.py
	reviewboard/diffviewer/tests/test_myersdiff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been dropped. Show all issues

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!

~		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.
	~	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.
	~	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.
	~	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.
	~	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.