flake8
-
reviewboard/attachments/managers.py (Diff revision 1) -
reviewboard/webapi/resources/filediff.py (Diff revision 1) line too long (80 > 79 characters) Column: 80 Error code: E501
Review Request #13538 — Created Feb. 12, 2024 and submitted
The
get_for_filediff
method for getting file attachments attached to
diffs had a few major issues that were preventing it from functioning
correctly.First, it was querying solely based on the repository, file path, and
revision. This meant we could theoretically end up returning
FileAttachment
objects which were attached to a different review
request than the givenFileDiff
. While our database schema
theoretically would allow for a single FileAttachment to be attached to
multiple review requests, in practice everything assumes that it's more
of a foreign key relationship than a many to many relationship.Second, when a review request is created with commit history, there are
actually multiple FileDiffs that all go into a single changed file. At
the very least, we'll have a FileDiff for the commit and a FileDiff for
the cumulative diff.When we're posting changes from RBTools, we attach the FileAttachment to
the FileDiff corresponding to the commit. This meant that even though
the file was uploaded, it wasn't visible in the diff viewer because we'd
usually be looking at the cumulative diff.This change makes it so
FileAttachment.objects.get_for_filediff
will
first try the simple case, and if that isn't found, it will check to see
if the DiffSet is created with commit history. If that's the case, we'll
look for the most recent FileDiff object corresponding to the given file
and use that instead.Finally, we mostly were relying on this function to raise
ObjectDoesNotExist
, but we also had a case where it could return
None
. I've changed it to always returnNone
when there are no
matching file attachments.
Summary | ID |
---|---|
22281d659f2f85975cbf8130d486136ac20358d4 |
Description | From | Last Updated |
---|---|---|
Can we update unit tests for this, given that there are some notable fixes in here? |
chipx86 | |
line too long (80 > 79 characters) Column: 80 Error code: E501 |
reviewbot | |
line too long (80 > 79 characters) Column: 80 Error code: E501 |
reviewbot | |
This function is missing a final explicit return None. It only has an implicit one. In fact, if we have … |
chipx86 | |
Tiny tiny nit, but we should consistently access using .pk instead of .id. They're technically interchangeable, but .pk is considered … |
chipx86 |
reviewboard/attachments/managers.py (Diff revision 1) |
---|
reviewboard/webapi/resources/filediff.py (Diff revision 1) |
---|
line too long (80 > 79 characters) Column: 80 Error code: E501
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+144 -54) |
reviewboard/attachments/managers.py (Diff revision 2) |
---|
This function is missing a final explicit
return None
. It only has an implicit one.In fact, if we have one, we can eliminate a few returns.
reviewboard/attachments/managers.py (Diff revision 2) |
---|
Tiny tiny nit, but we should consistently access using
.pk
instead of.id
. They're technically interchangeable, but.pk
is considered the "true" field for the primary key.
Commits: |
|
|||||||
---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+146 -56) |