flake8
-
reviewboard/datagrids/columns.py (Diff revision 1) Show all issues -
-
Review Request #10277 — Created Oct. 25, 2018 and updated
This fix makes the
FullNameColumn
column of the/users/
page
sortable. Previously, onlyUsernameColumn
was sortable.In order to implement this, an override method for
get_sort_field()
was added toFullNameColumn
which returns thefirst_name
and
last_name
as a tuple. A related change in the Djblets side enables
handling of tuples as returns fromget_sort_field()
.Two unit tests were added to test sorting of the test users in both
ascending and descending order using custom first and last names.
All tests pass for Review Board, except "Testing PerforceTool.connect
sets required client args", which can be disregarded for now.
Description | From | Last Updated |
---|---|---|
You do not need Review Board: in your summary since it will land in RB (and shows repository: Review Board … |
|
|
Your code only affects the FullNameColumn, so I would suggest slightly re-wording the summary to make that clear, e.g. Make … |
|
|
I think the first sentence of your description is a bit confusing (I had to read it a couple times … |
|
|
E303 too many blank lines (2) |
![]() |
|
E501 line too long (90 > 79 characters) |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
First line of Returns needs to be tuple: |
|
|
We can probably get rid of this test. There's no logic besides a fixed return value in get_sort_field, and it … |
|
|
No period at the end of unit test docstrings. |
|
|
""" should be on the previous line. Also, no period at the end of unit test docstrings. |
|
|
A few things of note here: The context object here is technically a ContextList object, lets you access individual keys … |
|
|
Same docstring comments apply here. |
|
|
F841 local variable 'fixtures' is assigned to but never used |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
Since we're making this column sortable as well, there should be a unit test testing this as well. That way, … |
|
|
It might be more clear to simply say Retrieve the first name and last name fields. |
|
|
A 2-tuple containing the fields to use to sort a user by their full name. |
|
|
It might be more clear to simply say Tuple containing the first name and last name fields. |
|
|
It might be clearer to say "Testing sorting of FullNameColumn in ascending order" |
|
reviewboard/datagrids/columns.py (Diff revision 1) |
---|
Added 10278 as a dependency for this fix.
Depends On: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+18 -1) |
Added unit tests for sort on ascending and descending order.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+64 -1) |
Description update.
Description: |
|
---|
reviewboard/datagrids/tests.py (Diff revision 3) |
---|
We can probably get rid of this test. There's no logic besides a fixed return value in
get_sort_field
, and it won't change unless we want it to, in which case this test also has to be changed. Tests that test something that can be impacted by other changes are more valuable.
reviewboard/datagrids/tests.py (Diff revision 3) |
---|
"""
should be on the previous line.Also, no period at the end of unit test docstrings.
reviewboard/datagrids/tests.py (Diff revision 3) |
---|
A few things of note here:
- The context object here is technically a
ContextList
object, lets you access individual keys without worrying about which sub-context it's in. So when we know what the exact key is that we want to look up, we can just grab it.- It's not really safe or efficient to loop through all items in a context. We can get way too many false-positives through that. So that ties into #1 as well.
- In general, it's better to collapse nested
if
statements like this, but we probably don't need that first one if we follow #1 and #2.What object are those
if
statements meant to compare with? If we pull that out and check it, this will be a much simpler, faster, safer test.
Fixing Christian's suggestions
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+52 -1) |
Using custom users for the unit tests.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+75 -1) |
reviewboard/datagrids/tests.py (Diff revision 5) |
---|
F841 local variable 'fixtures' is assigned to but never used
Added the test_users back in because they are set globally by
BaseColumnTestCase
.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+87 -1) |
Took the test_users right back out. All tests pass.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+94 -1) |
Now out of WIP.
Summary: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
Reverted, now updating test_users' first_name and last_name instead of deleting them or creating new users.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+68 -1) |
The
reviewboard/datagrids/tests.py
file runs all tests successfully, when using the associated djblets patch.
reviewboard/datagrids/grids.py (Diff revision 8) |
---|
Since we're making this column sortable as well, there should be a unit test testing this as well. That way, you know that it's sorting the way you expect it to.
Remove sorting on pending column - this was for my testing and is not required for this patch.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+67 -1) |
You do not need
Review Board:
in your summary since it will land in RB (and showsrepository: Review Board
in details).
Your code only affects the
FullNameColumn
, so I would suggest slightly re-wording the summary to make that clear, e.g.Make sotrable the full name column on user datagrids
This is because another column may be added in the future that may not be sortable and could lead to a confusing
git blame
.
reviewboard/datagrids/columns.py (Diff revision 9) |
---|
A 2-tuple containing the fields to use to sort a user by their full name.
I think the first sentence of your description is a bit confusing (I had to read it a couple times to make sense of it, I think because you split up the sentence with the names of the three columns so by the time I get to "sortable" I forgot what the subject of the sentence was).
You could simplify this to be "This fix makes all three columns of the /users/ page sortable by first and last name (the columns include
UsernameColumn
,FullNameColumn
andPendingCountColumn
)."
reviewboard/datagrids/columns.py (Diff revision 9) |
---|
It might be more clear to simply say
Retrieve the first name and last name fields.
reviewboard/datagrids/columns.py (Diff revision 9) |
---|
It might be more clear to simply say
Tuple containing the first name and last name fields.
reviewboard/datagrids/tests.py (Diff revision 9) |
---|
It might be clearer to say "Testing sorting of FullNameColumn in ascending order"
Summary: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|