Update the types for diff chunks and lines.

Review Request #14399 — Created April 14, 2025 and updated — Latest diff uploaded

Information

Review Board
master

Reviewers

This change defines a TypedDict for diff chunks, and a tuple for
diff lines, with definitions and documentation for each element.

Before, these were just an opaque dictionary, and a heterogenous list.
The diff lines were particularly annoying because they were variable
length (if there was no metadata, that element was just left off the
end), and types were annoying (for "real" line numbers, we'd have an
int if the line was present on that side of the diff, but an empty
string if not). This change technically breaks compatibility in those
two cases.

We now always include the metadata field, set to None if there's no
metadata for the line. This increases the cached size of a very large
diff with thousands of lines by less that 0.1% (much better than many
years ago with older pickle protocols--the tuple is probably also
helping here).

In the case of real line numbers which are not present on one side of
the diff (for example, the line number of an added line for the
left-hand side of the diff), we set the data for that to None. In
order to maintain compatibility with current versions of Review Bot, we
change None line numbers back to an empty string for the filediff
resource's diff_data mode.

This change only touches code related to chunks and lines. Other diff
viewer cleanup is coming in later changes.

  • Tested a ton of different diffs, both in the diff viewer as well as
    the text and markdown Review UIs. Verified that all types of edits
    (added/edited/deleted/moved) lines all worked as expected.
  • Ran unit tests.

Changes between revision 1 and 2

orig
1
2
3
4
5
6
7
8
9

Commits

Summary ID Author
Update the types for diff chunks and lines.
This change defines a `TypedDict` for diff chunks, and a `NamedTuple` for diff lines, with definitions and documentation for each element. Before, these were just an opaque dictionary, and a heterogenous list. The diff lines were particularly annoying because they were variable length (if there was no metadata, that element was just left off the end), and types were annoying (for "real" line numbers, we'd have an int if the line was present on that side of the diff, but was set to an empty string if it wasn't). This change technically breaks compatibility in those two cases. We now always include the metadata field, set to `None` if there's no metadata for the line. In the case of real line numbers which are not present on one side of the diff (for example, the line number of an added line for the left-hand side of the diff), we set the data for that to `None`. The `diff_lines` template tag and other consumers now use names instead of indexing magically into the structure, making it much clearer what's going on. This change only touches code related to chunks and lines. Other diff viewer cleanup is coming in later changes. Testing Done: - Tested a ton of different diffs, both in the diff viewer as well as the text and markdown Review UIs. Verified that all types of edits (added/edited/deleted/moved) lines all worked as expected. - Ran unit tests.
c07cac59fee7c3f20506e9999d70f54eb99148c6 David Trowbridge
Update the types for diff chunks and lines.
This change defines a `TypedDict` for diff chunks, and a `NamedTuple` for diff lines, with definitions and documentation for each element. Before, these were just an opaque dictionary, and a heterogenous list. The diff lines were particularly annoying because they were variable length (if there was no metadata, that element was just left off the end), and types were annoying (for "real" line numbers, we'd have an int if the line was present on that side of the diff, but was set to an empty string if it wasn't). This change technically breaks compatibility in those two cases. We now always include the metadata field, set to `None` if there's no metadata for the line. In the case of real line numbers which are not present on one side of the diff (for example, the line number of an added line for the left-hand side of the diff), we set the data for that to `None`. The `diff_lines` template tag and other consumers now use names instead of indexing magically into the structure, making it much clearer what's going on. This change only touches code related to chunks and lines. Other diff viewer cleanup is coming in later changes. Testing Done: - Tested a ton of different diffs, both in the diff viewer as well as the text and markdown Review UIs. Verified that all types of edits (added/edited/deleted/moved) lines all worked as expected. - Ran unit tests.
541e055e3978c23ea3bba8f9ee0b67c26fc4134f David Trowbridge
reviewboard/diffviewer/chunk_generator.py
reviewboard/diffviewer/templatetags/difftags.py
reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py
reviewboard/webapi/tests/test_filediff.py
Loading...