Return binary flag when with filediffs and allow querying by the binary flag.

Review Request #10729 — Created Sept. 26, 2019 and updated

perplex
Review Board
master
reviewboard, students

RB Tools required getting the binary status of each filediff so it
could determine which files need an accompanying file upload. This
allows RB Tools to query the backend for the filediffs for a diff. The
filediffs returned will contain the binart flag. An optional flag is
also provide for querying only for filediffs that have the binary flag
set to either true or false.

Included unit test for these changes.

Summary
Address David's comments
Fix grammar mistake
Revert "Fix grammar mistake"
Fix grammar
Add binary filter test
replace double quotes with single quotes
Use djblets to map query value to parameter
Add binary parameter to function description
drop get_queryset parameters done one line
format get_queryset description to fit line length limit
Description From Last Updated

Typo in summary: "amd"

brenniebrennie

This is including some strange delete/re-add of some changes. Can you look into what happened, try to revert those?

chipx86chipx86

Can you flesh out the description and testing done? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea

daviddavid

Can you add unit tests for both the query parameter and the new field in the output?

daviddavid

Let's keep these in alphabetical order

daviddavid

This needs a period at the end.

daviddavid

Please use single quotes rather than double.

daviddavid

This can be simplified down to: qs = qs.filter(binary=(param == 'true'))

daviddavid

"is binary" or "is a binary file".

brenniebrennie

This should be updated to talk about the new binary query parameter.

daviddavid

Instead of doing this, you can add a binary=False keyword arugment to get_queryset() and the API field will do the …

brenniebrennie

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot
david
  1. 
      
  2. Can you flesh out the description and testing done? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea

  3. reviewboard/webapi/resources/filediff.py (Diff revision 1)
     
     
     
     
     
     

    Let's keep these in alphabetical order

  4. This needs a period at the end.

  5. reviewboard/webapi/resources/filediff.py (Diff revision 1)
     
     
     

    Please use single quotes rather than double.

  6. reviewboard/webapi/resources/filediff.py (Diff revision 1)
     
     
     
     
     

    This can be simplified down to:

    qs = qs.filter(binary=(param == 'true'))
    
  7. 
      
PE
PE
brennie
  1. 
      
  2. Typo in summary: "amd"

  3. "is binary" or "is a binary file".

  4. reviewboard/webapi/resources/filediff.py (Diff revision 2)
     
     
     
     

    Instead of doing this, you can add a binary=False keyword arugment to get_queryset() and the API field will do the rest.

  5. 
      
david
  1. 
      
  2. Can you add unit tests for both the query parameter and the new field in the output?

  3. reviewboard/webapi/resources/filediff.py (Diff revision 2)
     
     
     

    This should be updated to talk about the new binary query parameter.

  4. 
      
PE
Review request changed

Summary:

-Return binary flag when with filediffs amd allow querying by the binary flag.
+Return binary flag when with filediffs and allow querying by the binary flag.

Commits:

Summary
-
Address David's comments
+
Address David's comments
+
Fix grammar mistake

Diff:

Revision 3 (+5911 -5893)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

chipx86
  1. 
      
  2. This is including some strange delete/re-add of some changes. Can you look into what happened, try to revert those?

  3. 
      
PE
Review request changed

Commits:

Summary
-
Address David's comments
-
Fix grammar mistake
+
Address David's comments
+
Fix grammar mistake
+
Revert "Fix grammar mistake"
+
Fix grammar

Diff:

Revision 4 (+5913 -5895)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
Review request changed

Commits:

Summary
-
Address David's comments
-
Fix grammar mistake
-
Revert "Fix grammar mistake"
-
Fix grammar
+
Address David's comments
+
Fix grammar mistake
+
Revert "Fix grammar mistake"
+
Fix grammar
+
Add binary filter test
+
replace double quotes with single quotes

Diff:

Revision 5 (+13587 -13451)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
Review request changed

Commits:

Summary
-
Address David's comments
-
Fix grammar mistake
-
Revert "Fix grammar mistake"
-
Fix grammar
-
Add binary filter test
-
replace double quotes with single quotes
+
Address David's comments
+
Fix grammar mistake
+
Revert "Fix grammar mistake"
+
Fix grammar
+
Add binary filter test
+
replace double quotes with single quotes
+
Use djblets to map query value to parameter

Diff:

Revision 6 (+13609 -13455)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
Review request changed

Commits:

Summary
-
Address David's comments
-
Fix grammar mistake
-
Revert "Fix grammar mistake"
-
Fix grammar
-
Add binary filter test
-
replace double quotes with single quotes
-
Use djblets to map query value to parameter
+
Address David's comments
+
Fix grammar mistake
+
Revert "Fix grammar mistake"
+
Fix grammar
+
Add binary filter test
+
replace double quotes with single quotes
+
Use djblets to map query value to parameter
+
Add binary parameter to function description
+
drop get_queryset parameters done one line

Diff:

Revision 7 (+13614 -13458)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
Review request changed

Commits:

Summary
-
Address David's comments
-
Fix grammar mistake
-
Revert "Fix grammar mistake"
-
Fix grammar
-
Add binary filter test
-
replace double quotes with single quotes
-
Use djblets to map query value to parameter
-
Add binary parameter to function description
-
drop get_queryset parameters done one line
+
Address David's comments
+
Fix grammar mistake
+
Revert "Fix grammar mistake"
+
Fix grammar
+
Add binary filter test
+
replace double quotes with single quotes
+
Use djblets to map query value to parameter
+
Add binary parameter to function description
+
drop get_queryset parameters done one line
+
format get_queryset description to fit line length limit

Diff:

Revision 8 (+13618 -13460)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
Review request changed

Description:

~  

RB Tools required getting the binary status of each filediff so it could determine which files need an accompanying file upload. This allows RB Tools to query the backend for the filediffs for a diff. The filediffs returned will contain the binart flag. An optional flag is also provide for querying only for filediffs that have the binary flag set to either true or false.

  ~

RB Tools required getting the binary status of each filediff so it

  + could determine which files need an accompanying file upload. This
  + allows RB Tools to query the backend for the filediffs for a diff. The
  + filediffs returned will contain the binart flag. An optional flag is
  + also provide for querying only for filediffs that have the binary flag
  + set to either true or false.

Testing Done:

  +

Included unit test for these changes.

Loading...