Refactoring the alphabetic paginator changes into new AlphanumericDatagrid subclass.

Review Request #6521 — Created Oct. 28, 2014 and submitted

Information

Djblets
master
829
03f56e7...

Reviewers

Changing the implementation of the alphabetic paginator to a subclass of the Datagrid: AlphanumericDatagrid.
The paginator(s) are no longer rendered using template tags, but instead a render_paginator function, which uses settings that are overloaded by the alphanumericDataGrid.

Unit tests all pass
All button cases manually tested. (Forward, back, first and last arrows, each letter).
Verified that changes to not affect any other module that inherits from datagrid (such as the groups list, or the all review requests list, the current 'master' of reviewboard with no changes to the userGrid).

Description From Last Updated

Col: 80 E501 line too long (99 > 79 characters)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Alphabetical order.

chipx86chipx86

Summary should be on the first line, like: """Renders the paginator ... ... """

chipx86chipx86

I don't recall what the original exception handler was set up for, but I think it was because custom columns …

chipx86chipx86

Docstrings should be in proper sentences, with trailing periods. We should also have a bit more detail in this particular …

chipx86chipx86

These blank lines should be removed.

chipx86chipx86

This should take only the arguments needed, and pass the rest as *args, **kwargs. Like: def __init__(self, request, sortable_column, *args, …

chipx86chipx86

This (and the one below) would be nicer as: queryset = queryset.filter(**{ sortable_column + '...': ..., })

chipx86chipx86

We use parens for multi-line conditions, with "or" on the previous line, like: elif (self.current_letter.isdecimal() or .....):

chipx86chipx86

Let's define this regex as a constant on the class, and use ' instead of ".

chipx86chipx86

Col: 47 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Make sure sentences end in periods. There must be a blank line between the summary and body.

chipx86chipx86

No blank line here.

chipx86chipx86

Must be on one line, or various doc builders will cut it off.

chipx86chipx86

Is this still used? If not, it can be removed.

chipx86chipx86

'string' imported but unused

reviewbotreviewbot

Can you put 0 first instead of last?

daviddavid

Would this be always true since you are always grabbing the letter A on line 1130?

AD adrw.hong
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (99 > 79 characters)
    
  3. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  5. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  6. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
chipx86
  1. Looking good! A few small things.

  2. djblets/datagrid/grids.py (Diff revision 2)
     
     
     

    Alphabetical order.

  3. djblets/datagrid/grids.py (Diff revision 2)
     
     
     

    Summary should be on the first line, like:

    """Renders the paginator ...
    
    ...
    """
    
  4. djblets/datagrid/grids.py (Diff revision 2)
     
     
     
     
     
     
     
     

    I don't recall what the original exception handler was set up for, but I think it was because custom columns themselves may end up breaking things, and we wanted some indication.

    The paginator shouldn't break, so we should be able to remove the try/except.

  5. djblets/datagrid/grids.py (Diff revision 2)
     
     

    Docstrings should be in proper sentences, with trailing periods. We should also have a bit more detail in this particular docstring about what advantages it offers.

  6. djblets/datagrid/grids.py (Diff revision 2)
     
     
     
     
     
     

    These blank lines should be removed.

  7. djblets/datagrid/grids.py (Diff revision 2)
     
     

    This should take only the arguments needed, and pass the rest as *args, **kwargs. Like:

    def __init__(self, request, sortable_column, *args, **kwargs):
        ...
    
        super(AlphanumericDataGrid, self).__init__(request, *args, **kwargs)
    
  8. djblets/datagrid/grids.py (Diff revision 2)
     
     
     

    This (and the one below) would be nicer as:

    queryset = queryset.filter(**{
        sortable_column + '...': ...,
    })
    
  9. djblets/datagrid/grids.py (Diff revision 2)
     
     
     

    We use parens for multi-line conditions, with "or" on the previous line, like:

    elif (self.current_letter.isdecimal() or
          .....):
    
  10. djblets/datagrid/grids.py (Diff revision 2)
     
     

    Let's define this regex as a constant on the class, and use ' instead of ".

  11. 
      
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. djblets/datagrid/grids.py (Diff revision 4)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
chipx86
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 6)
     
     
     
     

    Make sure sentences end in periods.

    There must be a blank line between the summary and body.

  3. djblets/datagrid/grids.py (Diff revision 6)
     
     
     
     

    No blank line here.

  4. djblets/datagrid/grids.py (Diff revision 6)
     
     
     

    Must be on one line, or various doc builders will cut it off.

  5. djblets/datagrid/templatetags/datagrid.py (Diff revision 6)
     
     
     
     

    Is this still used? If not, it can be removed.

  6. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2.  'string' imported but unused
    
  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
david
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 8)
     
     

    Can you put 0 first instead of last?

  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/datagrid/grids.py
        djblets/datagrid/templatetags/datagrid.py
    
    Ignored Files:
        djblets/datagrid/templates/datagrid/listview.html
        djblets/datagrid/templates/datagrid/paginator.html
        djblets/datagrid/templates/datagrid/alphanumeric_paginator.html
        djblets/datagrid/templates/datagrid/alphabetic_paginator.html
        djblets/datagrid/templates/datagrid/numeric_paginator.html
    
    
  2. 
      
AD
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 9)
     
     

    Would this be always true since you are always grabbing the letter A on line 1130?

    1. request.GET.get('letter', 'A') checks the URL for a querystring '?letter=ARG' and returns ARG. If no argument is provided, it defaults to 'A'.

  3. 
      
david
  1. Ship It!
  2. 
      
RM
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (4d49079)
Loading...