Spell-checking with options and contextmenu

Review Request #2207 — Created March 19, 2011 and discarded

Information

Review Board
spellcheck

Reviewers

- Spell-checking: highlight spell errors in changed lines.

- Options: spell-checking, personal wordlist path options under settings/diffs

- ContextMenu: ignore or add spell errors into personal wordlist with context menu on diff view page.
FireFox 3.6
Chromium 10.0
david
  1. 
      
  2. reviewboard/templates/diffviewer/view_diff.html (Diff revision 1)
     
     
     
     
     
     
     
    Indentation here is a little inconsistent.
  3. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Please keep this alphabetized.
  4. 
      
chipx86
  1. Cool stuff. Some work needed though to get it in shape.
  2. reviewboard/diffviewer/filters.py (Diff revision 1)
     
     
    Not a very useful description. I'd leave it out.
  3. reviewboard/diffviewer/filters.py (Diff revision 1)
     
     
    Should probably be:
    
    super(SpellCheckerWithPWL, self).__init__(lang, ...)
    
    In fact, since we're just mostly using values that SpellChecker takes, you can do:
    
    
    def __init__(self, *args, **kwargs):
        super(SpellCheckerWithPWL, self).__init__(*args, **kwargs)
    
    Then you'll need to access pwl and lang, which I imagine you can get from 'self'? If not, include just those in the argument lists and what you pass to the parent constructor.
  4. reviewboard/diffviewer/filters.py (Diff revision 1)
     
     
    We can't really rely on this path. It won't be writable on a public server, and it's shared, which means it'll be problematic with multiple Review Board installs on one server.
    
    This will need to be created on demand (see my other post about this). It might be worth having a configurable field for the location of this.
    
    The file should, by default, live in the site's data/ directory. You can see some examples for building a valid location by looking at what we do for the logging settings.
  5. reviewboard/diffviewer/filters.py (Diff revision 1)
     
     
     
    Might be good to check specifically for a comment here?
  6. reviewboard/htdocs/media/rb/js/spellcheck.js (Diff revision 1)
     
     
     
     
     
     
     
     
    I know it's a little bit more work, but all API communication should really happen through datastore.js APIs. This one should be simple, but you should have some representation in there.
  7. reviewboard/htdocs/media/rb/js/spellcheck.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
    The braces style isn't correct. It should be:
    
    if (o.className == 's_spellerr') {
        o.className = 's';
    } else {
        o.className = 'c';
    }
    
    Actually, you can even simplify this:
    
    o.className = (o.className == 's_spellerr' ? 's' : 'c');
  8. reviewboard/htdocs/media/rb/js/spellcheck.js (Diff revision 1)
     
     
     
     
     
     
     
    Same here about the datastore.js APIs.
  9. Need spaces around braces, like:
    
    function(t) { ignoreOnce(t); }
  10. Same here.
  11. reviewboard/templates/diffviewer/view_diff.html (Diff revision 1)
     
     
     
     
     
     
    One space indentation.
    
    "ignore" and "Add" are too generic. Please use "spellcheck_ignore" and "spellcheck_add"
    
    Also, {% trans "Ignore Once" %} and same for Add to Dictionary.
  12. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
    You'll need the proper decorators for this, like on other resources.
    
    @webapi_check_local_site
    @webapi_check_login_required
    @webapi_request_fields(...) with 'check'
    
    Also should have documentation for how this is used from an API standpoint.
  13. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    check can just be in the argument list to this function.
  14. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
    Need @webapi_check_local_site
  15. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Should have documentation on how this is used from an API standpoint.
  16. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    I'd just use 'word", and it can be directly in the argument list.
  17. 
      
DO
DO
DO
DO
DO
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
     
    No blank line here.
  3. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
    dict is a reserved word, so you'll need to use something else.
    
    spell_dict?
  4. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
    Sentence casing.
  5. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
     
    You can speed this up even more by pre-compiling the regex.
    
    Near the top of the file (we have other regexes there), you can do:
    
    SPELL_CHECKED_SPANS_RE = re.compile('<span ....')
    
    Then:
    
    mark = SPELL_CHECKED_SPANS_RE.search(line[5])
    
    Also, variable should just be 'm' for consistency, instead of 'mark'. I'll refer to it as 'm' in following comments.
    
    I'm a little concerned about this not finding more than one spelling error per line, though.
  6. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
    If you change the above regex to:
    
    '<span class="[sc]">(^<]*)</span>'
    
    Then you can just do:
    
    check = m.group(1)
  7. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
    Shouldn't this use check, instead of re-grabbing the range?
  8. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
    I'm not exactly sure I know what all you're doing here, but I suspect we can make it much cleaner. Mind going over it with me?
  9. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
    Probably best not to initialize these if we don't have spell checking enabled. They could just be set to None otherwise.
  10. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
    Spaces after each value in the list. Also, that should probably be a tuple.
  11. reviewboard/diffviewer/diffutils.py (Diff revision 4)
     
     
     
     
     
    Another speed improvement would be to have spell_check_lines 'yield' each item instead of returning an array.
    
    This is what's called a generator function. Instead of doing:
    
        def foo():
            lines = []
    
            for a in b:
                lines.append(something(a))
    
            return lines
    
    You can do:
    
        def foo():
            for a in b;
                 yield something(a)
    
    You'd then modify new_chunk to explicitly do: list(lines), which will convert it to a list when needed.
    
    The advantages are that 1) we don't have to use memory for this list in spell_check_lines, 2) we only do effectively one iteration instead of one in spell_check_lines and another in new_chunk.
  12. reviewboard/diffviewer/filters.py (Diff revision 4)
     
     
     
    Blank line between these.
  13. reviewboard/diffviewer/filters.py (Diff revision 4)
     
     
    You can just do:
    
    if pwl:
  14. reviewboard/diffviewer/filters.py (Diff revision 4)
     
     
    You probably want to initialize self.dict to None if we don't have a pwl.
  15. reviewboard/diffviewer/filters.py (Diff revision 4)
     
     
     
    This doesn't do what you may expect it to do. It's no different than doing: language = siteconfig.get(...).
    
    You basically never want to store global variables that can change. Only constants.
    
    I thought I had a review covering what to do here but I can't find it..
    
    Anyway, I think the purpose here is to not have to create SpellCheckers more than necessary? If so, here's what I suggest:
    
    1) Store a spell_checker_cache dictionary, with the key being the language and the value being a SpellChecker instance.
    
    2) Provide a get_spell_checker() function that takes the language and pwl. It will do:
    
        if language not in spell_checker_cache:
            spell_checker_cache[language] = SpellCheckerWithPWL(lang=language, pwl=pwl, filters=[...])
    
        return spell_checker_cache[language]
  16. No space in {}
  17. Space after the comma.
  18. No comma here. It will break Internet Explorer.
  19. Space before +
  20. No comma here. It will break Internet Explorer.
  21. No space before ;
    
    We don't actually want to set className to undefined, though, because an element can have multiple classes. Instead, specifically call $(word).removeClass('whatever').
  22. No comma here. It will break Internet Explorer.
  23. These two lines should be:
    
    var gSpellCheck = new RB.SpellCheck();
    
    I don't know that we want this in its own file though. I'd move it into diffviewer.js.
  24. This may not do what you expect. This only operates on existing elements with a "spellerr" class. Since we load diffs asynchronously, you're going to find it doesn't work on some diffs. Instead, look at jQuery's $.live()
  25. No comma here.
  26. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Space after :
  27. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
    Sentence casing and trailing period.
    
    I'd recommend: "The word to check in the spelling dictionary."
  28. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    "Returns information on a word."
  29. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
    "This includes whether or not the word is correctly spelled, and any spelling suggestions."
  30. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
     
    Indentation error with the """
  31. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    I think this should just be [] by default.
  32. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    "The word to add to the dictionary."
  33. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    "Adds a word to the dictionary."
    
    Note that it's not actually the logged in user's own dictionary.
  34. reviewboard/webapi/resources.py (Diff revision 4)
     
     
     
    "If the word is already added, this will return :http:`304`."
    
    (Note that the :http: thing is a special thing understood by our documentation generation code.)
  35. 
      
DO
david
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 5)
     
     
     
    You need to have a space after "have" before the quotation mark, otherwise when this gets concatenated it will say "... user wishes to havespell checking ..."
  3. reviewboard/admin/checks.py (Diff revision 5)
     
     
    I don't see this being used anywhere?
  4. reviewboard/admin/checks.py (Diff revision 5)
     
     
     
     
     
     
    Does this dict-style string formatting work in Python 2.4?
    1. Yeah, we're using this in other places.
  5. reviewboard/admin/forms.py (Diff revision 5)
     
     
     
    You need a space after the period before the quotation mark.
  6. reviewboard/admin/siteconfig.py (Diff revision 5)
     
     
     
    What happens if this is deployed on a system without an en_US dictionary installed?
  7. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Please call this get_enable_spell_checking. Elsewhere, when you use this, it's not obvious what "checking" means.
  8. 
      
DO
Review request changed
Status:
Discarded