Highlight Spell Errors for Spell Checking

Review Request #2122 — Created Feb. 12, 2011 and discarded

Information

Review Board
spell_check

Reviewers

Check lines with token type as "String" or "Comment" for spell errors, using pyaspell.
Highlight words with spell errors, using pygments.

 

Screenshots

chipx86
  1. Awesome start :)
    
    As is always the case with new contributors, there are style issues that need to be fixed. No big deal and they should be quick.
    
    Can you provide a screenshot of how it looks right now? You should be able to add it to the review request.
  2. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    There should be a blank line before this import.
    
    Imports are in three groups, each separated by a blank line: 1) Python-provided modules, 2) Third-party modules, 3) Project modules.
  3. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
     
     
     
     
    Can you put all these in alphabetical order?
  4. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Might be nicer to import token, and use token.String, etc.
  5. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Can you move this to a diffviewer.filters?
  6. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Since our constructor doesn't do anything, we can actually leave it out.
  7. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Spaces after each ","
    
    Also, functions should always be lowercase, with '_' between words. So, check_line.
  8. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Make sure the line is less than 80 characters
  9. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Same rule with variable names. spell_checker
  10. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Spaces on both sides of "="
  11. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Blank line between these.
  12. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    The indentation looks wrong.
  13. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    We should probably define a new token type: SpellingError. Then we can highlight that separately.
    
    I think you can do this with:
    
        from pygments import token
    
        ...
    
        SpellingError = Token.SpellingError
        token.STANDARD_TYPES[SpellingError] = 'spellerr'
    
    I haven't tried that, but it might work. We'd need a CSS rule for it, though, but that's easy.
  14. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Blank line between these.
  15. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Blank line between these too.
  16. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Trailing whitespace.
  17. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    Trailing whitespace.
  18. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
    We want both. codetagify should remain.
  19. 
      
DO
chipx86
  1. This looks awesome :) I'm so excited about this.
    
    A few things to fix up. Only one of them is a tough problem, the rest are easy changes. I'll give the localization thing below some thought... This must be a solved problem.
  2. reviewboard/diffviewer/filters.py (Diff revision 2)
     
     
    "A Filter"
  3. reviewboard/diffviewer/filters.py (Diff revision 2)
     
     
     
     
    Shouldn't need this, since it'll fall back to the parent __init__.
  4. reviewboard/diffviewer/filters.py (Diff revision 2)
     
     
     
     
     
     
    First line, immediately following the """, should contain a one-line summary, followed by a blank line, followed by a description.
  5. reviewboard/diffviewer/filters.py (Diff revision 2)
     
     
    So this is an interesting thing I didn't consider. We're defaulting to "en", which probably isn't sufficient for all cases. Imagine a Japanese or French developer putting up code and seeing spelling errors everywhere. We'll need to figure out a solution for this. Tricky.
  6. reviewboard/diffviewer/filters.py (Diff revision 2)
     
     
    Extra blank line at the end of the file should be removed.
  7. I'd say probably just .spellerr. Also, it should go in syntax.css.
  8. Two spaces for indentation.
  9. 
      
chipx86
  1. Oh, also, you'll need to add pyaspell to the dependency list in setup.py. Figure out what the latest version is and do: pyaspell>=the_version (assuming 'pyaspell' is what you easy_install).
  2. 
      
DO
DO
DO
DO
david
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 6)
     
     
     
     
    This doesn't really fit in the block, with regards to the try/except and comment.
    
    Since you're providing spellerror yourself, I'd just pull this line out of the block:
    
    lexer.add_filter('spellerror')
    try:
        # This is only available in 0.7 and higher
        lexer.add_filter('codetagify')
    except AttributeError:
        pass
  3. reviewboard/diffviewer/filters.py (Diff revision 6)
     
     
    Remove this line (enchant and pygments are both "3rd party" modules and go in the same block of imports).
  4. reviewboard/diffviewer/filters.py (Diff revision 6)
     
     
    Add a space between the comma and filters
  5. reviewboard/diffviewer/filters.py (Diff revision 6)
     
     
     
     
     
    Python lets you do this in one line via a "list comprehension"
    
    spell_errors = [err.word for err in spell_checker]
  6. reviewboard/diffviewer/filters.py (Diff revision 6)
     
     
     
    Since this is both fetching a tuple and returning that same tuple, you can just iterate over a single value without unpacking it:
    
    for val in self.check_line(ttype, value):
        yield val
  7. 
      
DO
DO
  1. 
      
  2. I'll delete this blank line.
  3. 
      
DO
Review request changed

Status: Discarded

Loading...