Add uploading of binary files for diffs.

Review Request #13571 — Created Feb. 23, 2024 and submitted

Information

RBTools
release-5.x

Reviewers

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.

  • 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.
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
Description From Last Updated

I think we should specify that this is an optional function to implement, and should only be implemeneted if the …

maubinmaubin

Missing a return type.

maubinmaubin

This can also raise a ValueError, could you add that while here.

maubinmaubin

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

maubinmaubin

Missing blank line.

chipx86chipx86

Must be explicitly typed bool, or it'll be Literal[False].

chipx86chipx86

Can we type this?

chipx86chipx86

Can we type this?

chipx86chipx86

Can this just be: files_to_upload += files.all_items If so, that'll simplify the code a bit and save some operations.

chipx86chipx86

Indentation is off by one level.

chipx86chipx86

'rbtools.api.resource.ItemResource' imported but unused Column: 5 Error code: F401

reviewbotreviewbot
david
maubin
  1. 
      
  2. rbtools/clients/base/scmclient.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    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?

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

  3. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues

    Missing a return type.

  4. rbtools/commands/post.py (Diff revision 2)
     
     
     
     
    Show all issues

    This can also raise a ValueError, could you add that while here.

  5. rbtools/commands/post.py (Diff revision 2)
     
     
    Show all issues

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

  6. 
      
david
chipx86
  1. 
      
  2. rbtools/clients/base/scmclient.py (Diff revision 3)
     
     
     
    Show all issues

    Missing blank line.

  3. rbtools/clients/base/scmclient.py (Diff revision 3)
     
     
    Show all issues

    Must be explicitly typed bool, or it'll be Literal[False].

  4. rbtools/commands/post.py (Diff revision 3)
     
     
    Show all issues

    Can we type this?

  5. rbtools/commands/post.py (Diff revision 3)
     
     
    Show all issues

    Can we type this?

  6. rbtools/commands/post.py (Diff revision 3)
     
     
     
    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.

  7. rbtools/commands/post.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    Indentation is off by one level.

  8. 
      
david
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
maubin
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.x (981bcbf)