Summary

Modernize the rest of chunk and opcode generators and differ base.

Review Request #14503 — Created July 11, 2025 and updated Sept. 9, 2025, 6:57 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change finishes up modernization of the chunk_generator,
differ, and opcode_generator files in the diffviewer. For the most
part this just involves fixing up docstrings and adding type hints.

The one functional change in here is a fix for a subtle bug that I
discovered when working through this: when computing headers, we were
subtracting 1 from the end line numbers of the chunk twice: once in
_new_chunk, and once in _get_interesting_headers. This meant we'd
stop iterating one line too soon. We obviously didn't notice any bugs in
practice, but it's possible that it was affecting the header
computation, especially for chunks that are only a single line.

Testing Done


Ran unit tests.
Smoke tested a bunch of diffs.

Commits

Summary	ID
Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	zwkyoovslsqxvknqzpzmtqmpummrntmn

Summary

Modernize the rest of chunk and opcode generators and differ base.

This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.

zwkyoovslsqxvknqzpzmtqmpummrntmn

Issues

Description	From	Last Updated
Let's make <div> a code literal.	chipx86	July 17, 2025, 11:24 p.m.
Might be a nice time to make these keyword-only.	chipx86	July 17, 2025, 11:25 p.m.
And here.	chipx86	July 17, 2025, 11:27 p.m.
Let's use str(). It's faster than an f-string that just turns into a str().	chipx86	July 17, 2025, 11:28 p.m.
This can now use the {..., **...} syntax.	chipx86	July 17, 2025, 11:28 p.m.
Mising a period (old bug).	chipx86	July 17, 2025, 11:29 p.m.
We might as well just append [1], since we're not using the variable.	chipx86	July 17, 2025, 11:29 p.m.
Raises comes after Returns.	chipx86	July 17, 2025, 11:29 p.m.
This needs to be the full class path.	chipx86	July 17, 2025, 11:29 p.m.
Not that anyone should hit this, but we might as well localize it.	chipx86	July 17, 2025, 11:30 p.m.
This can be simplified to: self.groups = groups or []	chipx86	July 17, 2025, 11:31 p.m.
Looks like there's an off-by-one alignment issue here.	chipx86	July 17, 2025, 11:31 p.m.
We can move the r_move_range = None to the else to avoid double-setting the variable.	chipx86	July 17, 2025, 11:32 p.m.
And this changed to keyword only args.	maubin	Sept. 9, 2025, 3:18 p.m.
Why can't we just give imeta a type of only dict[str, Any], since we always assert it to be non-None.	maubin	Sept. 9, 2025, 3:22 p.m.

flake8 passed.

JSHint passed.

Good cleanup. Most of this is just small cleanups/nits.

reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's make <div> a code literal.
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Might be a nice time to make these keyword-only.
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's use str(). It's faster than an f-string that just turns into a str().
```
reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This can now use the {..., **...} syntax.
```
reviewboard/diffviewer/differ.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Mising a period (old bug).
```
reviewboard/diffviewer/differ.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We might as well just append [1], since we're not using the variable.
```
reviewboard/diffviewer/differ.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Raises comes after Returns.
```
reviewboard/diffviewer/differ.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This needs to be the full class path.
```
reviewboard/diffviewer/differ.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Not that anyone should hit this, but we might as well localize it.
```

reviewboard/diffviewer/opcode_generator.py (Diff revision 1)

The issue has been resolved. Show all issues

This can be simplified to:

self.groups = groups or []

reviewboard/diffviewer/opcode_generator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Looks like there's an off-by-one alignment issue here.
```

reviewboard/diffviewer/opcode_generator.py (Diff revision 1)

The issue has been resolved. Show all issues

We can move the r_move_range = None to the else to avoid double-setting the variable.

Commits:

	Summary	ID
	Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	43e56320375f7e93bd17a44c99361c0e4cc8b3b9
	Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	7bdd856ae787f2f49ae22406b20643a5a01e58a1

Diff:

Revision 2 (+1390 -342)

Show changes

	reviewboard/deprecation.py
	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/differ.py
	reviewboard/diffviewer/opcode_generator.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	7bdd856ae787f2f49ae22406b20643a5a01e58a1
	Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	zwkyoovslsqxvknqzpzmtqmpummrntmn

Diff:

Revision 3 (+1390 -368)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/differ.py
	reviewboard/diffviewer/opcode_generator.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/chunk_generator.py (Diff revision 3)
The issue has been resolved. Show all issues
```
And this changed to keyword only args.
```

reviewboard/diffviewer/opcode_generator.py (Diff revision 3)

The issue has been dropped. Show all issues

Why can't we just give imeta a type of only dict[str, Any], since we always assert it to be non-None.

david Sept. 9, 2025, 3:22 p.m.

This is called by unpacking DiffOpcodeWithMetadata instances. For some opcodes (such as equal), it's valid to have None for the metadata field. For codepaths leading to this method, we will have always set a metadata dict, and so we assert here.

We can't change the type here without either updating DiffOpcodeWithMetadata to require a non-None metadata field (which is wasteful) or by unpacking and doing the assertion elsewhere.

Commits:

	Summary	ID
	Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	zwkyoovslsqxvknqzpzmtqmpummrntmn
	Modernize the rest of chunk and opcode generators and differ base. This change finishes up modernization of the `chunk_generator`, `differ`, and `opcode_generator` files in the diffviewer. For the most part this just involves fixing up docstrings and adding type hints. The one functional change in here is a fix for a subtle bug that I discovered when working through this: when computing headers, we were subtracting 1 from the end line numbers of the chunk twice: once in `_new_chunk`, and once in `_get_interesting_headers`. This meant we'd stop iterating one line too soon. We obviously didn't notice any bugs in practice, but it's possible that it was affecting the header computation, especially for chunks that are only a single line. Testing Done: - Ran unit tests. - Smoke tested a bunch of diffs.	zwkyoovslsqxvknqzpzmtqmpummrntmn

Diff:

Revision 4 (+1392 -368)

Show changes

	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/differ.py
	reviewboard/diffviewer/opcode_generator.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```