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)
     
     
    Show all issues
    Col: 80
     E501 line too long (99 > 79 characters)
    
  3. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (87 > 79 characters)
    
  5. djblets/datagrid/grids.py (Diff revision 1)
     
     
    Show all issues
    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)
     
     
     
    Show all issues

    Alphabetical order.

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

    Summary should be on the first line, like:

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

    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)
     
     
    Show all issues

    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)
     
     
     
     
     
     
    Show all issues

    These blank lines should be removed.

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

    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)
     
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues
    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)
     
     
     
     
    Show all issues

    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)
     
     
     
     
    Show all issues

    No blank line here.

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

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

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

    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. Show all issues
     '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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    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...