Update type for diff opcodes to be a NamedTuple.

Review Request #14398 — Created April 11, 2025 and updated

Information

Review Board
master

Reviewers

This change updates diff opcodes to use a NamedTuple with definitions
and documentation for each element.

We had one annoying, confusing thing here, which is that the opcodes
returned from the differ had 5 elements, and the opcodes returned from
the opcode generator had 6 (including an extra metadata dict). I've
changed it so everything operates on the same definition, with the
metadata dict being optional in earlier stages of the process. This does
mean we have a compatibility break here for the Differ classes, which
now yield opcodes as a 6-tuple instead of a 5-tuple. The only place
that did this outside of the chunk generator was in the change
description diffs for text-based review request fields.

In places where we process the opcodes, I've changed it to explicitly
use the names instead of unpacking in the loop handler. This means that
even where we're continuing to use variables like i1 and i2, we have
an initial assignment of those that make it clearer what those variables
actually are.

There's a lot of other cleanup that's going to happen in the diffviewer
code, but I wanted to keep this particular change completely separate.
Please don't worry about adjacent code that's unrelated to opcodes.

  • Did a bunch of smoke testing of diff generation and saw that
    everything still worked correctly.
  • Ran unit tests.
Summary ID
Update type for diff opcodes to be a NamedTuple.
This change updates diff opcodes to use a NamedTuple with definitions and documentation for each element. We had one annoying, confusing thing here, which is that the opcodes returned from the differ had 5 elements, and the opcodes returned from the opcode generator had 6 (including an extra metadata dict). I've changed it so everything operates on the same definition, with the metadata dict being optional in earlier stages of the process. This does mean we have a compatibility break here for the Differ classes, which now yield opcodes as a 6-tuple instead of a 5-tuple. The only place that did this outside of the chunk generator was in the change description diffs for text-based review request fields. In places where we process the opcodes, I've changed it to explicitly use the names instead of unpacking in the loop handler. This means that even where we're continuing to use variables like `i1` and `i2`, we have an initial assignment of those that make it clearer what those variables actually are. There's a lot of other cleanup that's going to happen in the diffviewer code, but I wanted to keep this particular change completely separate. Please don't worry about adjacent code that's unrelated to opcodes. Testing Done: - Did a bunch of smoke testing of diff generation and saw that everything still worked correctly. - Ran unit tests.
e8f545ef529682136fa7007941302a08ec809899
david
Review request changed
Commits:
Summary ID
Update type for diff opcodes to be a NamedTuple.
This change updates diff opcodes to use a NamedTuple with definitions and documentation for each element. We had one annoying, confusing thing here, which is that the opcodes returned from the differ had 5 elements, and the opcodes returned from the opcode generator had 6 (including an extra metadata dict). I've changed it so everything operates on the same definition, with the metadata dict being optional in earlier stages of the process. This does mean we have a compatibility break here for the Differ classes, which now yield opcodes as a 6-tuple instead of a 5-tuple. The only place that did this outside of the chunk generator was in the change description diffs for text-based review request fields. In places where we process the opcodes, I've changed it to explicitly use the names instead of unpacking in the loop handler. This means that even where we're continuing to use variables like `i1` and `i2`, we have an initial assignment of those that make it clearer what those variables actually are. There's a lot of other cleanup that's going to happen in the diffviewer code, but I wanted to keep this particular change completely separate. Please don't worry about adjacent code that's unrelated to opcodes. Testing Done: - Did a bunch of smoke testing of diff generation and saw that everything still worked correctly. - Ran unit tests.
7e657f53a8687f66341942fe88051eff41d85797
Update type for diff opcodes to be a NamedTuple.
This change updates diff opcodes to use a NamedTuple with definitions and documentation for each element. We had one annoying, confusing thing here, which is that the opcodes returned from the differ had 5 elements, and the opcodes returned from the opcode generator had 6 (including an extra metadata dict). I've changed it so everything operates on the same definition, with the metadata dict being optional in earlier stages of the process. This does mean we have a compatibility break here for the Differ classes, which now yield opcodes as a 6-tuple instead of a 5-tuple. The only place that did this outside of the chunk generator was in the change description diffs for text-based review request fields. In places where we process the opcodes, I've changed it to explicitly use the names instead of unpacking in the loop handler. This means that even where we're continuing to use variables like `i1` and `i2`, we have an initial assignment of those that make it clearer what those variables actually are. There's a lot of other cleanup that's going to happen in the diffviewer code, but I wanted to keep this particular change completely separate. Please don't worry about adjacent code that's unrelated to opcodes. Testing Done: - Did a bunch of smoke testing of diff generation and saw that everything still worked correctly. - Ran unit tests.
e8f545ef529682136fa7007941302a08ec809899

Checks run (2 succeeded)

flake8 passed.
JSHint passed.