Summary

Add new resources, part 4/7.

Review Request #14377 — Created March 20, 2025 and submitted April 30, 2025, 9:06 a.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This change adds the following new resource implementations:
- DiffCommentItemResource
- DiffCommentListResource
- GeneralCommentItemResource
- GeneralCommentListResource
- ReviewItemResource
- ReviewListResource
- ReviewReplyItemResource
- ReviewReplyListResource
- ScreenshotCommentItemResource
- ScreenshotCommentListResource

Testing Done


Ran unit tests.
Used the new resources from a test script and saw that everything

  worked as expected.

Commits

Summary	ID
Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	6fba8adcbfcb7cd6a29fdda4d9e3f36e9cee76bd

Summary

Add new resources, part 4/7.

This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.

6fba8adcbfcb7cd6a29fdda4d9e3f36e9cee76bd

Issues

Description	From	Last Updated
too many blank lines (2) Column: 5 Error code: E303	reviewbot	March 20, 2025, 10:03 a.m.
blank line at end of file Column: 1 Error code: W391	reviewbot	March 20, 2025, 10:04 a.m.
This needs to be removed.	maubin	March 27, 2025, 9:46 a.m.
This needs to be removed.	maubin	March 27, 2025, 9:46 a.m.
This needs to be removed.	maubin	March 27, 2025, 9:46 a.m.
This needs to be removed.	maubin	March 27, 2025, 9:46 a.m.
This needs to be removed.	maubin	March 27, 2025, 9:46 a.m.
This should be "GeneralCommentListResource"	maubin	March 27, 2025, 9:47 a.m.
Should say "list resource"	maubin	March 27, 2025, 9:47 a.m.
Just a suggestion, but might be nice to format this like: issue_status: Optional[Literal[ 'dropped', 'open', 'resolved', ... ]] Keeps it …	chipx86	April 1, 2025, 8:09 a.m.
Typo: contont -> content	chipx86	April 1, 2025, 8:10 a.m.
Feel this reads better as "... only to the owner ..."	chipx86	April 1, 2025, 8:10 a.m.
We should be explicit about what the line number refers to (the line of the generated diff, not of a …	chipx86	April 1, 2025, 8:11 a.m.
These should be swapped.	chipx86	April 1, 2025, 8:11 a.m.
Typo: "when" -> "that".	chipx86	April 1, 2025, 8:12 a.m.
Can we list the version range? Same below.	chipx86	April 1, 2025, 8:56 a.m.
"file attachment"? Same below.	chipx86	April 1, 2025, 8:13 a.m.
This probably applies broadly, but given the "if present," I feel like I as the reader need to know what …	chipx86	April 1, 2025, 9:04 a.m.
"file attachment"	chipx86	April 1, 2025, 8:18 a.m.
This isn't in the function signature.	chipx86	April 30, 2025, 9:05 a.m.
Here, too.	chipx86	April 30, 2025, 9:05 a.m.

flake8 failed.

JSHint passed.

flake8

rbtools/api/resource/diff_comment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
too many blank lines (2)

Column: 5
Error code: E303
```
rbtools/api/resource/screenshot_comment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
blank line at end of file

Column: 1
Error code: W391
```

Commits:

	Summary	ID
	Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	6a041c1f0a2bc870ac34a3f681abd66d66502d89
	Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	94851572f3f4d2c91b7a638332db91d467f5cd81

Diff:

Revision 2 (+4174 -16)

Show changes

	rbtools/api/resource/__init__.py
	rbtools/api/resource/base_comment.py
	rbtools/api/resource/base_review.py
	rbtools/api/resource/diff_comment.py
	rbtools/api/resource/file_attachment.py
	rbtools/api/resource/file_attachment_comment.py
	rbtools/api/resource/file_diff.py
	rbtools/api/resource/general_comment.py
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/api/resource/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs to be removed.
```
rbtools/api/resource/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs to be removed.
```
rbtools/api/resource/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs to be removed.
```
rbtools/api/resource/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs to be removed.
```
rbtools/api/resource/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs to be removed.
```
rbtools/api/resource/root.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be "GeneralCommentListResource"
```
rbtools/api/resource/screenshot.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should say "list resource"
```

Commits:

	Summary	ID
	Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	94851572f3f4d2c91b7a638332db91d467f5cd81
	Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	2ed3c0a7cc644a6610a15aa3a4548faaa9f14270

Diff:

Revision 3 (+4144 -16)

Show changes

	rbtools/api/resource/__init__.py
	rbtools/api/resource/base_comment.py
	rbtools/api/resource/base_review.py
	rbtools/api/resource/diff_comment.py
	rbtools/api/resource/file_attachment.py
	rbtools/api/resource/file_attachment_comment.py
	rbtools/api/resource/file_diff.py
	rbtools/api/resource/general_comment.py
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

rbtools/api/resource/base_comment.py (Diff revision 3)

The issue has been resolved. Show all issues

Just a suggestion, but might be nice to format this like:

issue_status: Optional[Literal[
    'dropped',
    'open',
    'resolved',
    ...
]]

Keeps it a bit more readable, and easier to keep in a nice alphabetical order.

rbtools/api/resource/base_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Typo: contont -> content
```
rbtools/api/resource/base_review.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Feel this reads better as "... only to the owner ..."
```

rbtools/api/resource/diff_comment.py (Diff revision 3)

The issue has been resolved. Show all issues

We should be explicit about what the line number refers to (the line of the generated diff, not of a file).

Same below.

rbtools/api/resource/file_attachment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These should be swapped.
```
rbtools/api/resource/file_attachment_comment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Typo: "when" -> "that".
```
rbtools/api/resource/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we list the version range? Same below.
```
rbtools/api/resource/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"file attachment"?
Same below.
```

rbtools/api/resource/root.py (Diff revision 3)

The issue has been dropped. Show all issues

This probably applies broadly, but given the "if present," I feel like I as the reader need to know what I should expect if it's not present. The exceptions look like actual errors, and this doesn't return None.

david April 1, 2025, 9:05 a.m.

Hmm, let me think about this. Something being not found results in an APIError, which is documented in this method, but maybe there's a way to make that clearer across the board.
Will add this to my notes for future changes.

rbtools/api/resource/root.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"file attachment"
```

Commits:

	Summary	ID
	Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	2ed3c0a7cc644a6610a15aa3a4548faaa9f14270
	Add new resources, part 4/7. This change adds the following new resource implementations: - DiffCommentItemResource - DiffCommentListResource - GeneralCommentItemResource - GeneralCommentListResource - ReviewItemResource - ReviewListResource - ReviewReplyItemResource - ReviewReplyListResource - ScreenshotCommentItemResource - ScreenshotCommentListResource Testing Done: - Ran unit tests. - Used the new resources from a test script and saw that everything worked as expected.	6fba8adcbfcb7cd6a29fdda4d9e3f36e9cee76bd

Diff:

Revision 4 (+4236 -68)

Show changes

	rbtools/api/resource/__init__.py
	rbtools/api/resource/base_comment.py
	rbtools/api/resource/base_review.py
	rbtools/api/resource/diff_comment.py
	rbtools/api/resource/file_attachment.py
	rbtools/api/resource/file_attachment_comment.py
	rbtools/api/resource/file_diff.py
	rbtools/api/resource/general_comment.py
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

rbtools/api/resource/root.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This isn't in the function signature.
```
rbtools/api/resource/root.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Here, too.
```

Status:: Completed
Change Summary:: Pushed to master (da259d3)