Summary

Comment serialization cleanup part 6: Fix terrible DiffFile names

Review Request #13660 — Created March 21, 2024 and submitted April 10, 2024, 1:20 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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:

The DiffFile model is pretty much an implementation detail of our
diffviewer UI. It seems extremely unlikely that anybody would be
digging into this.
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.

Commits

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

Summary

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

Issues

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 …	chipx86	April 2, 2024, 3:12 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	March 21, 2024, 10:19 p.m.
I do want to document versioning of any fields that have changed, because from time to time changes do need …	chipx86	April 2, 2024, 8:32 a.m.
Should this use import type?	chipx86	April 2, 2024, 8:32 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/diffutils.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	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.	3a87c86d7761ec135d9364e8d0e011f998097f2f
	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.	493987e97a9b36abdd91b6dbb5bb9849c20c1ca1

Diff:

Revision 2 (+382 -282)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_reviews_diff_viewer_view.py
	reviewboard/reviews/views/diff_fragments.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/static/rb/js/reviews/models/diffFileModel.ts
	reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been dropped. Show all issues

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.

david April 1, 2024, 10:38 p.m.

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

reviewboard/static/rb/js/reviews/models/diffFileModel.ts (Diff revision 2)

The issue has been resolved. Show all issues

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.

reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Should this use import type?
```

Commits:

	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.	493987e97a9b36abdd91b6dbb5bb9849c20c1ca1
	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

Diff:

Revision 3 (+432 -284)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/tests/test_reviews_diff_viewer_view.py
	reviewboard/reviews/views/diff_fragments.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/static/rb/js/reviews/models/diffFileModel.ts
	reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts
	reviewboard/static/rb/js/reviews/views/diffFileIndexView.ts
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (81869f0)