Tool: Pyflakes
Processed Files:
    rbintegrations/idonethis/forms.py
    rbintegrations/extension.py
    rbintegrations/idonethis/integration.py
    rbintegrations/idonethis/utils.py
    rbintegrations/idonethis/tests.py
    rbintegrations/idonethis/pages.py

Ignored Files:
    rbintegrations/static/images/idonethis/icon@2x.png
    rbintegrations/idonethis/__init__.py
    rbintegrations/static/images/idonethis/icon.png
    rbintegrations/templates/rbintegrations/idonethis/account_page_form.html



Tool: PEP8 Style Checker
Processed Files:
    rbintegrations/idonethis/forms.py
    rbintegrations/extension.py
    rbintegrations/idonethis/integration.py
    rbintegrations/idonethis/utils.py
    rbintegrations/idonethis/tests.py
    rbintegrations/idonethis/pages.py

Ignored Files:
    rbintegrations/static/images/idonethis/icon@2x.png
    rbintegrations/idonethis/__init__.py
    rbintegrations/static/images/idonethis/icon.png
    rbintegrations/templates/rbintegrations/idonethis/account_page_form.html

Col: 19
 E127 continuation line over-indented for visual indent

This doc comment should include a "Returns" section.

This doc comment should include a "Returns" section.

Because this is a property, the docstring should look more like it is for attributes (such as just "The icons used for the integration") instead of the docstring for a method.

Add a trailing comma.

Unlike all our other docstrings, test case descriptions shouldn't include a period, because the test runner adds an ellipsis when printing. Here and throughout this file.

Would you mind changing these to use the format strings instead of a hard-coded result? These tests are fragile to text changes.

Trailing comma.

Trailing comma.

Trailing comma. I assume the same throughout this file.

Trailing comma.

Because this is imported into other code as just create_request, it was initially a bit confusing when I read that code. Can we rename this as create_idt_request so it's clear what's going on?

File-level docstring.

<b> is deprecated. Use <strong> instead.

The fieldset label should be marked for translation.

Here too.

Maybe use <p> over linebreaks? Also, IDK if we use self-closing tags (they are deprecated in the HTML spec)

Missing file-level docstring.

Must inherit from object.

unicode.format is slow compared to using %-based interpolation (and we also don't really use the former elsewhere). You can change these to use:


format_strings = {
    REPLY_PUBLISHED:
        'Replied to review request %(request_id)s: '
        '%(summary)s %(url)s %(group_tags)s',

    #...
}


and then interpolate with:

format_strings[entry] % {
    'group_tags': group_tags,
    'num_issues': num_issues,
    'request_id': review_request.display_id,
    'summary': review_request.summary,
    'url': url,
}

You may want to replace - with _ in group.name because idt doesn't like tags with dashes (but underscores work for some reason :/)

Why not just use, e.g.:


import re
#...

re.sub(r'\s{2,}', ' ', entry)


You can compile the regex with re.compile so we only have to parse the regex once.
I'm not sure which is more expensive

Coalesce into elif

File-level docstring.

File-level docstring.

File-level docstring.

Sorry this took so long :( Change looks great, and I love the test coverage. Most of my stuff are small doc tweaks and other tiny suggestions.

This should be "Format".

Text like ${num_issues} should be surrounded with double backticks so it'll appear as code/literal text in docs (not that we generate them yet, but still).
Same with others in this function.

We should use review_request_id instead of request_id in places. In our terminology, "Request" is the HTTP request.

I couple suggestions for formatting and optimizations:

group_names = review_request.target_groups.values_list('name', flat=True)

group_tags = ' '.join(
    '#%s' % re.sub(r'\W', '_', group_name)
    for group_name in group_names
)


We should also ideally precompile the regex in the module.

Let's precompile this one too.

"ID"

Let's put the URL in <code>...</code>.

"ID"

We should maybe expand this to say why this might be the case (the API token may be incorrect, please check the token in your account, etc.).

You can reference by doing:
:py:mod:`rbintegrations.idonethis.entries`

Might want to explicitly disregard None entries in the conditional instead of doing the lookup.

I'm generally a fan of collapsing down if statements, but given that we have these same checks within the if review.ship_it, maybe these ones should go in an else.

Wonder if we should just do the build_server_url in post_entry, so each call site doesn't have to reepeat it.

Should be "reply".

"Return ..."
"Get" implies that it's getting it from somewhere else, and doesn't necessarily imply that it's returning it.

No need for the settings.get if it's known to be in there.
Since it's most likely in there, you can actually do:

try:
    return decrypt_password(settings['api_token'])
except KeyError:
    return None

Same as above.

"IDs"

No blank line here.

We probably should turn this into two different paragraphs, or just let it wrap, rather than forcing a line break.

This is looking pretty solid!

This should be models.review_request.ReviewRequest. You can wrap it after any period.

Maybe wrap this as:

raise forms.ValidationError(
    ugettext('Team ID cannot contain slashes.'))

This looks great!
Was there anything else you were planning for this change? If not, can you add the binary files (icons) as file attachments to this review request?