Summary

Add uploading of binary files for diffs.

Review Request #13571 — Created Feb. 23, 2024 and submitted March 5, 2024, 9:23 a.m.

Information

Owner

david

Repository

RBTools

Branch

release-5.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This change adds the rbt post logic for uploading binary files to
diffs. We start by assembling a list of the newly-created FileDiffs that
are marked as binary (using the new binary flag to the FileDiff API).
This is either done for the individual commits (when posting with commit
history) or for the full diff (when posting a squashed diff or on tools
that do not support history). We then go through and find what files we
should upload, filtering by whether it's a reviewable MIME type and the
configured file limit. We then create new file attachments for each
matching binary file.

This includes the base SCMClient flag and method definitions, but does
not implement upload for any SCMs. We have to do it per client because
we need to add new APIs to the clients to get file content (and
optionally file size) at specific revisions. Individual per-SCM
implementations will come in future changes.

Testing Done


Ran unit tests.
Posted a whole bunch of changes with Git and SVN repositories that

  included image files. Tested with both commit history and squashed

  diffs.

Commits

Summary	ID
Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	40704b1378f6ea09fb4201aece271f0111371cff

Summary

Add uploading of binary files for diffs.

This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.

40704b1378f6ea09fb4201aece271f0111371cff

Issues

Description	From	Last Updated
I think we should specify that this is an optional function to implement, and should only be implemeneted if the …	maubin	Feb. 26, 2024, 1:43 p.m.
Missing a return type.	maubin	Feb. 26, 2024, 1:44 p.m.
This can also raise a ValueError, could you add that while here.	maubin	Feb. 26, 2024, 1:45 p.m.
Even though this and _upload_binary_files are private functions, might as well add "Version Added" to the docstrings.	maubin	Feb. 26, 2024, 1:45 p.m.
Missing blank line.	chipx86	Feb. 27, 2024, 10:34 a.m.
Must be explicitly typed bool, or it'll be Literal[False].	chipx86	Feb. 27, 2024, 10:34 a.m.
Can we type this?	chipx86	Feb. 27, 2024, 10:36 a.m.
Can we type this?	chipx86	Feb. 27, 2024, 10:36 a.m.
Can this just be: files_to_upload += files.all_items If so, that'll simplify the code a bit and save some operations.	chipx86	Feb. 27, 2024, 10:36 a.m.
Indentation is off by one level.	chipx86	Feb. 27, 2024, 10:37 a.m.
'rbtools.api.resource.ItemResource' imported but unused Column: 5 Error code: F401	reviewbot	Feb. 27, 2024, 10:47 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Add missing Returns section in some docstrings.

Commits:

	Summary	ID
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	c150ebac3ce065c841fb6f6900b1d6238797cb04
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	f04883dc59b03bf07b14b927febe1b95743a3b4d

Diff:

Revision 2 (+592 -98)

Show changes

	rbtools/clients/base/scmclient.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/base/scmclient.py (Diff revision 2)

The issue has been resolved. Show all issues

I think we should specify that this is an optional function to implement, and should only be implemeneted if the subclass's tool can get the file size without loading the file.
Or, would it be better to put the logic of loading the file and getting the file size (which currently exists in _upload_binary_files) as a default here, and then specify that subclasses should optimize this if possible?

david Feb. 26, 2024, 1:46 p.m.

I don't think it makes sense to move that logic here because then we'd end up loading the file twice.

rbtools/commands/post.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a return type.
```
rbtools/commands/post.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This can also raise a ValueError, could you add that while here.
```

rbtools/commands/post.py (Diff revision 2)

The issue has been resolved. Show all issues

Even though this and _upload_binary_files are private functions, might as well add "Version Added" to the docstrings.

Commits:

	Summary	ID
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	f04883dc59b03bf07b14b927febe1b95743a3b4d
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	b3d467c76d4fc4f84a546aadf22186e6d5b4ad17

Diff:

Revision 3 (+628 -98)

Show changes

	rbtools/clients/base/scmclient.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/base/scmclient.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing blank line.
```
rbtools/clients/base/scmclient.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Must be explicitly typed bool, or it'll be Literal[False].
```
rbtools/commands/post.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we type this?
```
rbtools/commands/post.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we type this?
```

rbtools/commands/post.py (Diff revision 3)

The issue has been resolved. Show all issues

Can this just be:

files_to_upload += files.all_items

If so, that'll simplify the code a bit and save some operations.

rbtools/commands/post.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Indentation is off by one level.
```

Commits:

	Summary	ID
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	b3d467c76d4fc4f84a546aadf22186e6d5b4ad17
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	26d5bfcbd93c028c78f635b8886380c5be6713a5

Diff:

Revision 4 (+630 -98)

Show changes

	rbtools/clients/base/scmclient.py
	rbtools/commands/post.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/post.py (Diff revision 4)

The issue has been dropped. Show all issues

'rbtools.api.resource.ItemResource' imported but unused

Column: 5
Error code: F401

Commits:

	Summary	ID
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	26d5bfcbd93c028c78f635b8886380c5be6713a5
	Add uploading of binary files for diffs. This change adds the `rbt post` logic for uploading binary files to diffs. We start by assembling a list of the newly-created FileDiffs that are marked as binary (using the new binary flag to the FileDiff API). This is either done for the individual commits (when posting with commit history) or for the full diff (when posting a squashed diff or on tools that do not support history). We then go through and find what files we should upload, filtering by whether it's a reviewable MIME type and the configured file limit. We then create new file attachments for each matching binary file. This includes the base SCMClient flag and method definitions, but does not implement upload for any SCMs. We have to do it per client because we need to add new APIs to the clients to get file content (and optionally file size) at specific revisions. Individual per-SCM implementations will come in future changes. Testing Done: - Ran unit tests. - Posted a whole bunch of changes with Git and SVN repositories that included image files. Tested with both commit history and squashed diffs.	40704b1378f6ea09fb4201aece271f0111371cff

Diff:

Revision 5 (+628 -98)

Show changes

	rbtools/clients/base/scmclient.py
	rbtools/commands/post.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-5.x (981bcbf)