Dashboard bugs column as clickable links to bug tracker system (if defined)

Review Request #3275 — Created Aug. 13, 2012 and discarded

Information

Review Board
main

Reviewers

Makes the bug numbers into clickable links if a bug tracker system is associated with the review.
Tested on local environment with and without a bug tracker template.
Description From Last Updated

Imports must be alphabetized.

chipx86chipx86

Two blank lines between classes.

chipx86chipx86

I know some older classes don't do this right, but this should be: super(Column, self).__init__(_('Bugs'), ...)

chipx86chipx86

No space before ':'

chipx86chipx86

You can actually simplify this with the very neat Python generators: return ','.join([ '%s' % (bug_url % bug, url) for …

chipx86chipx86

This should include the format, and ideally say "... when rendering bugs column". So: logging.warning('Invalid bug tracker format when rendering …

chipx86chipx86

Two blank lines.

chipx86chipx86

Alignment issues. Make sure you're not using tabs in any code. It should always be 4 space indentation.

chipx86chipx86

No need for the \, since it's in parens.

chipx86chipx86

We can just unconditionally return this at the end. No need to do it once in the exception handler and …

chipx86chipx86

Can you make the = align with the other columns?

chipx86chipx86

Alignment issue.

chipx86chipx86

Probably should be ", ", actually. A space will make it look nicer, and will allow it to wrap if …

chipx86chipx86

Blank line between these.

chipx86chipx86
chipx86
  1. This looks like a good addition! There's some style things to fix up, and there's a nice trick you can use, but the logic looks generally fine.
  2. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
    Imports must be alphabetized.
  3. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
    Two blank lines between classes.
  4. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    I know some older classes don't do this right, but this should be:
    
    super(Column, self).__init__(_('Bugs'), ...)
    1. I presume this should be: super(BugsColumn, self).__init__(_('Bugs'), ...) ?
    2. Oops, yes.
  5. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    No space before ':'
  6. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
     
     
    You can actually simplify this with the very neat Python generators:
    
    return ','.join([
        '<a href="%s">%s</a>' % (bug_url % bug, url)
        for bug in bugs
    ])
  7. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
    This should include the format, and ideally say "... when rendering bugs column". So:
    
    logging.warning('Invalid bug tracker format when rendering bugs column: %s' % bug_url)
  8. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
    Two blank lines.
  9. 
      
TC
chipx86
  1. 
      
  2. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
     
    Alignment issues.
    
    Make sure you're not using tabs in any code. It should always be 4 space indentation.
    1. I really ought to review PEP 8. I found a cool vim plugin that will help me out in the future at least.
  3. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
    No need for the \, since it's in parens.
    
    
  4. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
     
     
    We can just unconditionally return this at the end. No need to do it once in the exception handler and once in the else statement.
  5. reviewboard/reviews/datagrids.py (Diff revision 2)
     
     
    Can you make the = align with the other columns?
  6. 
      
TC
chipx86
  1. This looks good in general, but one thing I'd like to see is the links become blue and underlined in the dashboard, so it's clear they're clickable.
    
    You can have a class for the column and use that to key off the CSS.
  2. reviewboard/reviews/datagrids.py (Diff revision 3)
     
     
     
    Alignment issue.
  3. reviewboard/reviews/datagrids.py (Diff revision 3)
     
     
    Probably should be ", ", actually. A space will make it look nicer, and will allow it to wrap if there's a large number of bugs.
  4. reviewboard/reviews/datagrids.py (Diff revision 3)
     
     
     
    Blank line between these.
  5. 
      
TC
Review request changed

Status: Discarded

Loading...