Indentation here is a little inconsistent.

Please keep this alphabetized.

Cool stuff. Some work needed though to get it in shape.

Not a very useful description. I'd leave it out.

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.

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.

Might be good to check specifically for a comment here?

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.

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');

Same here about the datastore.js APIs.

Need spaces around braces, like:

function(t) { ignoreOnce(t); }

Same here.

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.

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.

check can just be in the argument list to this function.

Need @webapi_check_local_site

Should have documentation on how this is used from an API standpoint.

I'd just use 'word", and it can be directly in the argument list.

No blank line here.

dict is a reserved word, so you'll need to use something else.

spell_dict?

Sentence casing.

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.

If you change the above regex to:

'<span class="[sc]">(^<]*)</span>'

Then you can just do:

check = m.group(1)

Shouldn't this use check, instead of re-grabbing the range?

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?

Probably best not to initialize these if we don't have spell checking enabled. They could just be set to None otherwise.

Spaces after each value in the list. Also, that should probably be a tuple.

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.

Blank line between these.

You can just do:

if pwl:

You probably want to initialize self.dict to None if we don't have a pwl.

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]

No space in {}

Space after the comma.

No comma here. It will break Internet Explorer.

Space before +

No comma here. It will break Internet Explorer.

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

No comma here. It will break Internet Explorer.

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.

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()

No comma here.

Space after :

Sentence casing and trailing period.

I'd recommend: "The word to check in the spelling dictionary."

"Returns information on a word."

"This includes whether or not the word is correctly spelled, and any spelling suggestions."

Indentation error with the """

I think this should just be [] by default.

"The word to add to the dictionary."

"Adds a word to the dictionary."

Note that it's not actually the logged in user's own dictionary.

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

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

I don't see this being used anywhere?

Does this dict-style string formatting work in Python 2.4?

You need a space after the period before the quotation mark.

What happens if this is deployed on a system without an en_US dictionary installed?

Please call this get_enable_spell_checking. Elsewhere, when you use this, it's not obvious what "checking" means.