Summary

Convert comment block views to TypeScript/spina.

Review Request #13527 — Created Feb. 6, 2024 and submitted Feb. 20, 2024, 9:42 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change converts the comment block view classes to TypeScript.

Testing Done


Ran js-tests.
Smoke tested commenting on diffs and various Review UIs.

Commits

Summary	ID
Convert comment block views to TypeScript/spina. This change converts the comment block view classes to TypeScript. Testing Done: - Ran js-tests. - Smoke tested commenting on diffs and various Review UIs. Reviewed at https://reviews.reviewboard.org/r/13527/	b24b677ea58c0b35a7a0c9acd67924a6f203414f

Issues

Description	From	Last Updated
I notice there's a lot of places in the old code where we could pull out things like this.model and …	maubin	Feb. 14, 2024, 11:39 p.m.
To keep this formatted similarly to other classes and deal with the indent, you could do: export class DiffCommentBlockView extends …	chipx86	Feb. 15, 2024, 8:34 p.m.
This could be moved into static modelEvents = { ... }. If not that, then this should be changed to …	chipx86	Feb. 15, 2024, 8:36 p.m.
These are missing docs.	chipx86	Feb. 15, 2024, 8:38 p.m.
Missing Version Added.	chipx86	Feb. 16, 2024, midnight
These are missing docs.	chipx86	Feb. 16, 2024, midnight
These should be moved into modelEvents.	chipx86	Feb. 15, 2024, 8:41 p.m.
Let's pull #moveState into a local variable. No sense in looking it up 8 times.	chipx86	Feb. 15, 2024, 8:42 p.m.
Could use an f-string here.	chipx86	Feb. 15, 2024, 8:43 p.m.
Could use f-strings here.	chipx86	Feb. 15, 2024, 8:43 p.m.

flake8 passed.

JSHint passed.

Ship it!

The issue has been dropped. Show all issues

I notice there's a lot of places in the old code where we could pull out things like this.model and this.$el into variables to avoid repeated lookups. Not really critical to change this, but you could tackle it while you're here if you want.

david Feb. 14, 2024, 11:39 p.m.

I think this change is already big enough but I'll make a note of it.

reviewboard/static/rb/js/reviews/views/diffCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

To keep this formatted similarly to other classes and deal with the indent, you could do:

export class DiffCommentBlockView extends TextBasedCommentBlockView<
    DiffCommentBlock
> {
    ...
}

reviewboard/static/rb/js/reviews/views/abstractCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

This could be moved into static modelEvents = { ... }.
If not that, then this should be changed to use .listenTo() instead of .on().

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

These are missing docs.

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)

The issue has been dropped. Show all issues

Missing Version Added.

david Feb. 15, 2024, 8:44 p.m.

This is purely internal API within the RegionCommentBlockView. I don't think we care about documenting versions for that?

chipx86 Feb. 19, 2024, 7 p.m.

I think it's useful for code archaeology when trying to figure out when stuff changed. I try to document anything new, regardless if it's for internal or public consumption, so it's always clear.

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)

The issue has been dropped. Show all issues

These are missing docs.

david Feb. 15, 2024, 8:44 p.m.

So I'm fine adding explanations for the types as a whole, but these interfaces and types are purely internal to RegionCommentBlockView, and really just exist so that we get type checking and consistency within the implementation. I don't think we gain anything by adding a bunch of comments that mostly just reflect the same name as the attributes.

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

These should be moved into modelEvents.

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

Let's pull #moveState into a local variable. No sense in looking it up 8 times.

reviewboard/static/rb/js/reviews/views/textBasedCommentBlockView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Could use an f-string here.
```
1. david Feb. 15, 2024, 8:44 p.m.
  This is TS, not Python, but I know what you meant.
2. chipx86 Feb. 19, 2024, 7 p.m.
  We're gonna have to find a Python-to-TS converter so I can make my comment make more sense.

reviewboard/static/rb/js/reviews/views/textBasedCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

Could use f-strings here.

Commits:

	Summary	ID
	Convert comment block views to TypeScript/spina. This change converts the comment block view classes to TypeScript. Testing Done: - Ran js-tests. - Smoke tested commenting on diffs and various Review UIs.	e48b266aa016358148347bc9ce08170a676be3ed
	Convert comment block views to TypeScript/spina. This change converts the comment block view classes to TypeScript. Testing Done: - Ran js-tests. - Smoke tested commenting on diffs and various Review UIs. Reviewed at https://reviews.reviewboard.org/r/13527/	b24b677ea58c0b35a7a0c9acd67924a6f203414f

Diff:

Revision 2 (+746 -416)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/reviews/index.ts
	reviewboard/static/rb/js/views/abstractCommentBlockView.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.es6.js
	reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
	reviewboard/static/rb/js/views/textBasedCommentBlockView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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