Improve Review Bot's admin interface

Review Request #6999 — Created Feb. 28, 2015 and discarded

Information

ReviewBot
master

Reviewers

As a part of the project "Improve Review Bot's admin interface" in https://reviewboard.hackpad.com/Student-Project-Ideas-bWzTgtBtq9f, this change improves UI in "Tool Change" view in admin site.

The styling of the form is done in https://reviews.reviewboard.org/r/6998/

Works done are:

  1. implemented JSONDictWidget
    this is the widget to be used for profile.tool_settings
  2. linked profile model admin with ProfileForest and ProfileForm
  3. made ProfileForm use JSONDictWidget for 'tool_settings' field

Now, tool_settings part is rendered as a set of widgets which are declared in "reviewbot/tools/{tool_name}.py" for each tool.

Browser test done:
1. go to http://localhost:8080/admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/tool/
2. click one of the tools. for example, "PEP8 Style Checker"
3. see if existing profiles' "Tool settings" part is displayed well
4. click "Add another profile"
5. fill out the form and save
6. click the same tool
7. see if newly added profile is displayed well
8. try deleting and check again

Some possible edge cases are:
1. there's no existing profiles
2. there's exactly one porfile


Description From Last Updated

'Field' imported but unused

reviewbotreviewbot

'Widget' imported but unused

reviewbotreviewbot

You should combine this with the other reviewbotext.forms import.

brenniebrennie

No comma on this line.

brenniebrennie

Move this to the previous line.

brenniebrennie

Move this to the previous line.

brenniebrennie

Blank line between these.

brenniebrennie

It seems very un-Pythonic to be doing very strict type checking.

brenniebrennie

You can do if class_path.

brenniebrennie

Why not just '.'.join(class_path) ? This will make another list.

brenniebrennie

This should be in the second import group.

brenniebrennie

Our imports are split into three groups: Python standard library imports Third party library imports Project library imports. Each import …

brenniebrennie

This should go in the third import group. It should also be an absolute import, e.g. from reviewbotext.utils import get_class

brenniebrennie

This should go in the first import group.

brenniebrennie

Add a docstring.

brenniebrennie

Move this to the previous line.

brenniebrennie

Period.

brenniebrennie

Period.

brenniebrennie

Use a period. Can this be clarified?

brenniebrennie

Comma on this line.

brenniebrennie

Period.

brenniebrennie

Is there any reason that we have to check if this is a Widget? Are we expecting to ever not …

brenniebrennie

Period.

brenniebrennie

Period.

brenniebrennie

Move this to the previous line.

brenniebrennie

You should do from django.utils.six.moves import zip The six zip creates a generator instead of a list.

brenniebrennie

Blank line between these.

brenniebrennie

String concatenation is expensive. You should store all the parts in an array and use ''.join(string_parts)

brenniebrennie

Move this to the previous line.

brenniebrennie

Period.

brenniebrennie

Period.

brenniebrennie

Use a generator expression instead of a list comprehension (i.e., use ( and ) instead of [ and ]. It …

brenniebrennie

We don't need the enclosing [ and ]; it is more efficient to use a generator expression than a list …

brenniebrennie

'Widget' imported but unused

reviewbotreviewbot

Can be in alphabetical order.

XU xuanyi

'Field' imported but unused

reviewbotreviewbot

Does one blank line after class docstring apply to this? I understand that most Widget class did have a blank …

XU xuanyi

Add trailing comma.

VT VTL-Developer

Leftover debug output?

daviddavid

Since this is now unused, we should just get rid of it.

daviddavid

typo: "defiend"

daviddavid

It might be nicer to do the join here, rather than below.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
  2. extension/reviewbotext/forms.py (Diff revision 1)
     
     
    Show all issues
     'Field' imported but unused
    
  3. extension/reviewbotext/forms.py (Diff revision 1)
     
     
    Show all issues
     'Widget' imported but unused
    
  4. 
      
YY
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
  2. 
      
YY
YY
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
  2. 
      
brennie
  1. 
      
  2. extension/reviewbotext/admin.py (Diff revision 3)
     
     
    Show all issues

    You should combine this with the other reviewbotext.forms import.

  3. extension/reviewbotext/forms.py (Diff revision 3)
     
     
    Show all issues

    No comma on this line.

  4. extension/reviewbotext/forms.py (Diff revision 3)
     
     
    Show all issues

    Move this to the previous line.

  5. extension/reviewbotext/forms.py (Diff revision 3)
     
     
    Show all issues

    Move this to the previous line.

  6. extension/reviewbotext/forms.py (Diff revision 3)
     
     
     
    Show all issues

    Blank line between these.

  7. extension/reviewbotext/utils.py (Diff revision 3)
     
     
    Show all issues

    It seems very un-Pythonic to be doing very strict type checking.

  8. extension/reviewbotext/utils.py (Diff revision 3)
     
     
    Show all issues

    You can do if class_path.

  9. extension/reviewbotext/utils.py (Diff revision 3)
     
     
    Show all issues

    Why not just '.'.join(class_path) ? This will make another list.

    1. it is to skip the last element

  10. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
     
    Show all issues

    This should be in the second import group.

  11. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Our imports are split into three groups:

    1. Python standard library imports
    2. Third party library imports
    3. Project library imports.

    Each import group should be separated by a blank line.

  12. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    This should go in the third import group. It should also be an absolute import, e.g.

    from reviewbotext.utils import get_class
    
  13. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    This should go in the first import group.

  14. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Add a docstring.

  15. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Move this to the previous line.

  16. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  17. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  18. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Use a period. Can this be clarified?

  19. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Comma on this line.

    1. Should I put comma for the last element as well?

    2. This is the last element?

    3. yes

  20. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  21. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Is there any reason that we have to check if this is a Widget? Are we expecting to ever not get a Widget passed in?

    This seems very un-Pythonic.

  22. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  23. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  24. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Move this to the previous line.

  25. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    You should do

    from django.utils.six.moves import zip
    

    The six zip creates a generator instead of a list.

  26. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
     
    Show all issues

    Blank line between these.

  27. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    String concatenation is expensive. You should store all the parts in an array and use

    ''.join(string_parts)
    
  28. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Move this to the previous line.

  29. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  30. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
    Show all issues

    Period.

  31. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    Use a generator expression instead of a list comprehension (i.e., use ( and ) instead of [ and ]. It doesn't require building a new list and you are just iterating over it.

  32. extension/reviewbotext/widgets.py (Diff revision 3)
     
     
     
     
     
    Show all issues

    We don't need the enclosing [ and ]; it is more efficient to use a generator expression than a list comprehension.

  33. 
      
YY
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
  2. extension/reviewbotext/widgets.py (Diff revision 4)
     
     
    Show all issues
     'Widget' imported but unused
    
  3. extension/reviewbotext/widgets.py (Diff revision 4)
     
     
    Show all issues
     'Field' imported but unused
    
  4. 
      
XU
  1. 
      
  2. extension/reviewbotext/widgets.py (Diff revision 4)
     
     
    Show all issues

    Can be in alphabetical order.

  3. extension/reviewbotext/widgets.py (Diff revision 4)
     
     
    Show all issues

    Does one blank line after class docstring apply to this? I understand that most Widget class did have a blank line.

  4. 
      
YY
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
  2. 
      
VT
  1. 
      
  2. extension/reviewbotext/forms.py (Diff revision 5)
     
     
     
     
     
     
     
     

    I don't know how often empty_form would get called or when it will get called. Maybe cache it after creation? An example to look at is reviewboard/hostingsvcs/models.py in reviewboard.

  3. extension/reviewbotext/widgets.py (Diff revision 5)
     
     
    Show all issues

    Add trailing comma.

    1. should i add comma to the last element as well?

  4. 
      
YY
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        extension/reviewbotext/utils.py
        extension/reviewbotext/forms.py
        extension/reviewbotext/widgets.py
        extension/reviewbotext/admin.py
    
    
  2. 
      
david
  1. 
      
  2. extension/reviewbotext/forms.py (Diff revision 6)
     
     
     
     
    Show all issues

    Leftover debug output?

  3. extension/reviewbotext/forms.py (Diff revision 6)
     
     
    Show all issues

    Since this is now unused, we should just get rid of it.

  4. extension/reviewbotext/widgets.py (Diff revision 6)
     
     
    Show all issues

    typo: "defiend"

  5. extension/reviewbotext/widgets.py (Diff revision 6)
     
     
    Show all issues

    It might be nicer to do the join here, rather than below.

  6. 
      
david
Review request changed
Status:
Discarded