Comment serialization cleanup part 6: Fix terrible DiffFile names

Review Request #13660 — Created March 21, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

The DiffFile model (and the files context key when rendering
diffviewer templates) still used variable names from the very first days
of Review Board, when we used a fair bit of Perforce terminology.

This change fixes that to use orig_filename, orig_revision,
modified_filename, and modified_revision. This is a breaking change,
but I think that's okay for two reasons:

  1. The DiffFile model is pretty much an implementation detail of our
    diffviewer UI. It seems extremely unlikely that anybody would be
    digging into this.
  2. The documentation inside DiffFile was quite incorrect for many of
    the attributes. This was fixed in a prior change, but reinforces my
    belief that nobody was using it.

Ran python tests.
Ran js-tests.

Summary ID
Comment serialization cleanup part 6: Fix terrible DiffFile names
The DiffFile model (and the `files` context key when rendering diffviewer templates) still used variable names from the very first days of Review Board, when we used a fair bit of Perforce terminology. This change fixes that to use `orig_filename`, `orig_revision`, `modified_filename`, and `modified_revision`. This is a breaking change, but I think that's okay for two reasons: 1. The `DiffFile` model is pretty much an implementation detail of our diffviewer UI. It seems extremely unlikely that anybody would be digging into this. 2. The documentation inside `DiffFile` was quite incorrect for many of the attributes. This was fixed in a prior change, but reinforces my belief that nobody was using it. Testing Done: Ran python tests. Ran js-tests.
c3569256e79e4322448ac3bd5165f09f675e0ed8
Description From Last Updated

This affects the Diff Context API, right? If so, then we do have to be careful with this, because it's …

chipx86chipx86

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

I do want to document versioning of any fields that have changed, because from time to time changes do need …

chipx86chipx86

Should this use import type?

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

flake8

david
chipx86
  1. 
      
  2. This affects the Diff Context API, right? If so, then we do have to be careful with this, because it's been used in the wild in a couple places in the past. Whether it's actively in use, I don't know, but we should be careful with it.

    If this does affect it, then we need to figure out a versioning scheme here. We can discuss that more offline.

    1. It has been? We've always been very clear in our docs that the diff context API is internal and unstable.

  3. I do want to document versioning of any fields that have changed, because from time to time changes do need to be backported and this history could easily be lost.

  4. Should this use import type?

  5. 
      
david
maubin
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (81869f0)
Loading...