Added login rate limiting feature in Djblets auth and web api

Review Request #8698 — Created Jan. 31, 2017 and submitted

Information

Djblets
master
7531c49...

Reviewers

Developed rate-limit for failed login attempts through either the UI
or API. See review request 8768 to see it's implementation in Review
Board's Authentication form.

Create several unit tests for the login rate limiting feature that
test the rate limits as either custom or default values. They are
found in djblets/auth/tests/test_ratelimit.py and in
webapi/tests/test_api_auth_backend.py.

Description From Last Updated

Care to wrap the description & testing done at 72 characters?

brenniebrennie

Col: 80 E501 line too long (97 > 79 characters)

reviewbotreviewbot

Col: 36 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 4 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 24 E231 missing whitespace after ','

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 9 E116 unexpected indentation (comment)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 68 W292 no newline at end of file

reviewbotreviewbot

Col: 36 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 80 E501 line too long (97 > 79 characters)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 4 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 24 E231 missing whitespace after ','

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 9 E116 unexpected indentation (comment)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

By calling the variable prep_cache_key, you are shadowing the function prep_cache_key.

brenniebrennie

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 68 W292 no newline at end of file

reviewbotreviewbot

Do we want to give the frequency or the minimum period between attempts? If you use minimum period you can …

brenniebrennie

Col: 36 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 80 E501 line too long (97 > 79 characters)

reviewbotreviewbot

'cache_memoize' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 4 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 61 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 52 W291 trailing whitespace

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 61 W291 trailing whitespace

reviewbotreviewbot

Col: 60 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

'Group' imported but unused

reviewbotreviewbot

'User' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 48 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 48 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 48 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Docstrings should use three double-quotes instead of single. The first line should also be joined with the quotes so the …

daviddavid

We should list the full type here (django.http.HttpRequest).

daviddavid

This should include the type: Returns: unicode: ...

daviddavid

We should use six.string_type(...) (importing django.utils.six above). On Python 2, str is a bytestring, while on Python 3, it's a …

daviddavid

Same here with the docstring. This should also be written in the imperative mood ("Return ..." instead of "Returns ...")

daviddavid

Should be "unicode" instead of "String"

daviddavid

Should list the return type here.

daviddavid

Same comment with docstrings.

daviddavid

Arg types should be unicode. This also needs a Returns.

daviddavid

Please add a blank line between each arg.

daviddavid

You don't need the u prefix here because you imported unicode_literals at the top.

daviddavid

Same docstring comments here.

daviddavid

Same docstring comments here.

daviddavid

Some of our old test code uses camelCase, but new code should use underscores, such as test_invalid_key

daviddavid

Same here.

daviddavid

Same here.

daviddavid

This can be self.assertFalse(result)

daviddavid

This can be self.assertTrue(result)

daviddavid

Col: 80 E501 line too long (95 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 64 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (92 > 79 characters)

reviewbotreviewbot

Col: 44 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 44 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 46 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 80 E501 line too long (96 > 79 characters)

reviewbotreviewbot

'settings_local' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

Col: 64 W291 trailing whitespace

reviewbotreviewbot

Col: 45 W291 trailing whitespace

reviewbotreviewbot

'os' imported but unused

reviewbotreviewbot

'settings_local' imported but unused

reviewbotreviewbot

'LOGIN_LIMIT_RATE' imported but unused

reviewbotreviewbot

undefined name 'dependency_error'

reviewbotreviewbot

'settings_local' imported but unused

reviewbotreviewbot

'LOGIN_LIMIT_RATE' imported but unused

reviewbotreviewbot

Blank line between these (this is in the djblets codebase, so djblets imports are "this module")

daviddavid

Since these are "constants", they should be ALL_CAPS

daviddavid

Please use single-quotes for strings.

daviddavid

This regex string should be prefixed by r (r'([...) to indicate that it's a "raw" string, so the \s behave …

daviddavid

Dedent this 4 spaces (we don't indent the definition because there's only ever one return value, unlike args where there …

daviddavid

Combine these so the docstring doesn't start with a blank line ("""Return number of ...)

daviddavid

This should list a single return value, "tuple of int". The description for that return value should say that it's …

daviddavid

If this fails to match, we should print a warning and return a default value.

daviddavid

Merge these lines.

daviddavid

Should be "unicode" instead of "String"

daviddavid

Dedent this 4 spaces.

daviddavid

Docstrings should use 3 double-quotes. Also, please merge these lines.

daviddavid

String -> unicode.

daviddavid

String -> unicode.

daviddavid

Should be: Returns: bool: Whether the current user has exceeded ...

daviddavid

Leftover debug code?

daviddavid

Three double-quotes, and the first line of the docstring should be a single-line summary on the same line as the …

daviddavid

String -> unicode.

daviddavid

String -> unicode.

daviddavid

Returns: dict: A dictionary containing ...

daviddavid

Leftover debug code?

daviddavid

Let's break this out into multiple lines: return { 'count': count, 'limit': limit, 'time_left': time_left, }

daviddavid

Blank line between these.

daviddavid

Let's create the requestFactory inside the test-case class, probably in a setUpClass method. That way it won't be done unless …

daviddavid

For the unit tests in this file, I think we can merge them all into one test class.

daviddavid

This docstring should go within the function instead of above it (right now it's acting as the docstring for the …

daviddavid

Test docstrings should be of the format "Testing X" to match the others in the suite output.

daviddavid

Test docstrings should be of the format "Testing X" to match the others in the suite output.

daviddavid

Test docstrings should be of the format "Testing X" to match the others in the suite output.

daviddavid

We shouldn't need/want this. settings_local is something we do in Review Board, but the djblets settings.py file is primarily there …

daviddavid

'settings_local' imported but unused

reviewbotreviewbot

'LOGIN_LIMIT_RATE' imported but unused

reviewbotreviewbot

Blank line between these two.

daviddavid

If I'm reading the code correctly, this will update the rate limit info in the cache, even if the authentication …

daviddavid

Leftover debug code?

daviddavid

"ck_ratelimit" is kind of a weird variable name. If you get rid of the debug output, we can just move …

daviddavid

This can wrap a little better to fit within the 80 columns. The second line should also align with the …

daviddavid

Let's rewrap this for better formatting: self.assertEqual( result, (False, 'Maximum number of login attempts exceeded', None))

daviddavid

Let's rewrap this for better formatting: self.assertEqual( result, (False, 'Maximum number of login attempts exceeded', None))

daviddavid

'settings_local' imported but unused

reviewbotreviewbot

'LOGIN_LIMIT_RATE' imported but unused

reviewbotreviewbot

Summary should be on first line.

brenniebrennie

We don't have return annotation support for multiple returns or namned returns. This technically returns a tuple. However, we can …

brenniebrennie

What happens if i set RATE_LIMIT = 'nope'? We should raise ImproperlyConfigured if we cannot parse this value.

brenniebrennie

Docstring.

brenniebrennie

You will not want to use naive times (i.e., without timezones). You will want to do: from django.utils import timezone …

brenniebrennie

Does w stand for window? If so, can we name it that?

brenniebrennie

Same comments re: summaries.

brenniebrennie

unicode.

brenniebrennie

This shouldn't be indented.

brenniebrennie

Double quotes for docstrings. Summary should be on this line. Also, docstrings should be in the imperitive mood, i.e. they …

brenniebrennie

Double quotes for docstrings. Summary should be on this line. Also, docstrings should be in the imperitive mood, i.e. they …

brenniebrennie

This is missing the type annotation (dict). You can also use restructuredText to annotate the keys and values. dict: A …

brenniebrennie

We won't want this to be a global. Instead we can put this inside the setUpClass method of RateLimitTests: class …

brenniebrennie

Instead of this, you can use django.contrib.auth.User and django.contrib.auth.AnonymousUser. The latter will always have is_authenticated() return False.

brenniebrennie

There should be a testcase that tests that improper data raises an exception.

brenniebrennie

Blank line between these. Missing period. Missing docstring for test method.

brenniebrennie

This should use self.assertEqual It should also probably include a message about the rate being tested. e.g. self.assertEqual(count_seconds, split_rate(rate), "split_rate(%r) …

brenniebrennie

RateLimitTests We make our test case class names plural

brenniebrennie

Missing period.

brenniebrennie

This must call super(RateLimitTests, self).setUp() Also, this will not clear the cache after the last test is run so add: …

brenniebrennie

test case names should indicate what feature they are testing. e.g. "Testing rate limit invalid keys" or similar. Also it …

brenniebrennie

Testing

brenniebrennie

Testing

brenniebrennie

settings_local is a Review Board thing. Undo this change.

brenniebrennie

'settings_local' imported but unused

reviewbotreviewbot

'LOGIN_LIMIT_RATE' imported but unused

reviewbotreviewbot

Single quotes.

brenniebrennie

No hanging indent here. It should be flush with the first "

brenniebrennie

This file should not be committed.

brenniebrennie

Col: 63 E225 missing whitespace around operator

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (109 > 79 characters)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 18 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 18 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 18 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 18 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 18 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 18 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 18 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 18 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Bare imports should be above from ...

chipx86chipx86

If this is meant to be public, it should have a comment using #: and a one-line summary. If it's …

chipx86chipx86

No need for [] around \d

chipx86chipx86

How about get_user_id_or_ip?

chipx86chipx86

Maybe mention that this is for the request here. Otherwise it sounds like it'd be generic enough for any users …

chipx86chipx86

Here and in other docstrings, you can fit a lot more per line (up to 79 characters).

chipx86chipx86

"String" is redundant (because of the type), so you can just say "The user ID or IP."

chipx86chipx86

This will want to also factor in two other possible variables often used when standing up a proxy in front …

chipx86chipx86

The name doesn't really make the purpose of this function clear. The docstring isn't quite correct either. What this is …

chipx86chipx86

If PERIOD is meant to be public (which it should be if we're referencing it), then it needs to be …

chipx86chipx86

Any literal (a variable or tuple or similar) should be using double backticks to turn it into an inline code …

chipx86chipx86

It feels like this shouldn't really be handled here. Instead, this function should focus solely on doing this conversion. Callers …

chipx86chipx86

No need to pre-define these. They'll always be set below.

chipx86chipx86

What exceptions are we expecting to hit?

chipx86chipx86

We never want to use print, as this can actually break the web server. Instead, logging statements should be used, …

chipx86chipx86

You can set this below in Rate(...) instead of doing it here.

chipx86chipx86

This can be private.

chipx86chipx86

Docstring summaries must be one line. You have a lot more room to work with on here though.

chipx86chipx86

I don't fully understand this method. It assumes knowledge of what a "window" means in this case. Perhaps you can …

chipx86chipx86

user_id_or_ip is a better variable.

chipx86chipx86

Blank line between these.

chipx86chipx86

int, not Integer. Same elsewhere.

chipx86chipx86

This line should only have the type (int).

chipx86chipx86

We may want time.gmtime here, so timezones aren't an issue?

chipx86chipx86

It'd be nice if you could document the algorithm here.

chipx86chipx86

This can be private.

chipx86chipx86

Same comment as above with the naming.

chipx86chipx86

This can be one statement. In fact, much of the above can be combined into one statement.

chipx86chipx86

For functions returning things, it's better to have get_ or is_ or can_ or similar prefixes (depending on the function). …

chipx86chipx86

Missing docs for increment.

chipx86chipx86

I don't really see value in allowing these to be provided. There's unlikely to be a case where we need …

chipx86chipx86

Blank line here.

chipx86chipx86

No blank line here.

chipx86chipx86

limited is too generic a thing to be tacking onto a request object. You would need to use something like …

chipx86chipx86

Parameters should come after get_usage_count like other functions.

chipx86chipx86

Must be one line.

chipx86chipx86

This is missing increment.

chipx86chipx86

Blank line here.

chipx86chipx86

The dictionary entries cannot be indented past the original text, or this will be displayed wrong.

chipx86chipx86

You shouldn't use a backslash in strings, or it'll look like: Could not understand ratelimit key Instead, you can just …

chipx86chipx86

Since this is the default in the code, do we need it here? Only unit tests and such will be …

chipx86chipx86

Col: 13 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

'namedtuple' imported but unused

reviewbotreviewbot

Col: 8 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 25 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 73 W291 trailing whitespace

reviewbotreviewbot

'zlib' imported but unused

reviewbotreviewbot

'ImproperlyConfigured' imported but unused

reviewbotreviewbot

Col: 68 W291 trailing whitespace

reviewbotreviewbot

Col: 17 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

'User' imported but unused

reviewbotreviewbot

'ImproperlyConfigured' imported but unused

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

'DEFAULT_KEY' imported but unused

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

Col: 29 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

Missing a period.

brenniebrennie

This information belongs in the Returns section.

brenniebrennie

Any reason to cast this to a string? The consumer makes it into a cache key, but returning an int …

brenniebrennie

typo: "THe"

brenniebrennie

No comma here.

brenniebrennie

Delete this line.

brenniebrennie

Can you rename this to timestamp? I assume thats what ts is short for.

brenniebrennie

Args does not match up with the actual argument list.

brenniebrennie

This can all be one line.

brenniebrennie

This can be one line.

brenniebrennie

Single line.

brenniebrennie

This should not be called DEFAULT_LIMIT, since it is just the limit. Only constants should be ALLCAPS.

brenniebrennie

Missing period.

brenniebrennie

Expand what ck stands for, please.

brenniebrennie

Add a trailing comma.

brenniebrennie

Missing a docstring.

brenniebrennie

This should be a constant on the class.

brenniebrennie

Compiled regexes are intended to be reused. By compiling it in the function, you only ever use it once (because …

brenniebrennie

Instead of this, how about: m = rate_re.match(rate_str) if m: count, multi, period = m.groups() else: logging.warn("...") count, multi, period …

brenniebrennie

This should be except Exception:. A empty except clause can also catch things you don't want to catch, such as …

brenniebrennie

This should be a constant either on the class or globally.

brenniebrennie

Use %r for the rate so that we get the actual representation. That way, if someone just passes an int …

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

self.__dict__.keys() is going to return more than you want to iterate over. For example: >>> class Foo(object): ... x = …

brenniebrennie

'override_settings' imported but unused

reviewbotreviewbot

This isn't used.

brenniebrennie

We generally use the format: Testing Rate.parse

brenniebrennie

Same here re: docstring.

brenniebrennie

This can be a single statement.

brenniebrennie

Include the method you're testing (is_ratelimited), e.g. """Testing is_ratelimited with invalid rate limit parameter"""

brenniebrennie

You should give it a pk= arg, because it won't have one by default until it gets saved to the …

brenniebrennie

This can be a singel statement.

brenniebrennie

Same here re: docstring.

brenniebrennie

You should give it a pk= arg, because it won't have one by default until it gets saved to the …

brenniebrennie

This can be a single statement.

brenniebrennie

This seems to be repeated from above.

brenniebrennie

This should be at the top of the class after setUpClass.

brenniebrennie

'settings' imported but unused

reviewbotreviewbot

Missing a period. This string should also probably be marked for translation with ugettext.

brenniebrennie

Are we sure we want to do this and not just return result here? Perhaps double check with David on …

brenniebrennie

Col: 13 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

Can you reflow your docstrings to 79 chars wide? They're unnecessarily narrow.

brenniebrennie

str is bytes, i.e. binary data. You want to use six.text_type here. However, you can just do: return '%s%s%s' % …

brenniebrennie

Unnecessary.

brenniebrennie

e.g. over ex. Missing period.

brenniebrennie

Variables usually should have noun names. How about prepped_cache_key?

brenniebrennie

Technically, is False :) Missing a period.

brenniebrennie

This comment is unnecessary.

brenniebrennie

Can we call this initial_value ?

brenniebrennie

Docstrings should be of the format: """Single line summary. Multi-line description. """

brenniebrennie

Blank line between these.

brenniebrennie

If multi is multiplier can we rename it to that?

brenniebrennie

Same comment here re: docstrings. Also missing args.

brenniebrennie

Missing a "Yields" section. How about: "Yield the count and seconds attributes attributes. Yields: ... """

brenniebrennie

Again, I don't think this is appropriate.

brenniebrennie

'override_settings' imported but unused

reviewbotreviewbot

Blank line between these.

brenniebrennie

Col: 67 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

Did you want this to be prefixed with _ as well? Doesn't seem to be used else where in this …

KH khp

I think this would be better on one line.

KH khp

Could be wrong here but I think we usually just put the return type and not the variable name under …

KH khp

Unless this is something else, integer types are called int, so probably best to use (int) instead of (integer) here.

KH khp

Same as above.

KH khp

Same as above, this could be a private method I think.

KH khp

Should this be at line 167?

KH khp

Just a suggestion, and not 100% sure if it's equivalent: # Initialize cache with cache_key if it doesn't exist already. …

KH khp

There's an extra newline I think.

KH khp

Extra newline here.

KH khp

int is better here I think. There's more below.

KH khp

Extra newline here.

KH khp

'override_settings' imported but unused

reviewbotreviewbot

This description is kind of confusing because the method doesn't do anything with keys or users. It's really just applying …

daviddavid

"the given key" implies that a key is passed in, which it's not.

daviddavid

Don't we want to add period back to this value? Based on my reading of this, it returns a "timestamp" …

daviddavid

You can remove the single-quotes from the name of the key here.

daviddavid

And here.

daviddavid

And here.

daviddavid

It seems very complex to define a class for this which has a classmethod to parse a string, instantiates and …

daviddavid

The first line in the docstring should be on the same line as the """

daviddavid

The first line in the docstring should be on the same line as the """

daviddavid

'override_settings' imported but unused

reviewbotreviewbot

The places that you use _ are within function bodies, so this should use ugettext instead of ugettext_lazy.

daviddavid

'override_settings' imported but unused

reviewbotreviewbot

'zlib' imported but unused

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

'override_settings' imported but unused

reviewbotreviewbot

"Utilities"

brenniebrennie

Can you document this with a #: doc comment ?

brenniebrennie

Reflow this.

brenniebrennie

Instead of "user_id_or_ip", "user ID or IP address"

brenniebrennie

get_usage_count will never not return a dict with count or limit, so you can just use plain old indexing: return …

brenniebrennie

wouldnt this be rate_limit ?

brenniebrennie

Missing period.

brenniebrennie

Summary should be on the first line. How about: A rate representing login attempt frequency. or similar.

brenniebrennie

Instead of this, why not just implement __eq__ to test for equality?

brenniebrennie

Missing file docstring.

brenniebrennie

'override_settings' imported but unused

reviewbotreviewbot

This docstring doesnt fit.

brenniebrennie

This doesn't need to be inside the with.

brenniebrennie

'django.test.utils.override_settings' imported but unused

reviewbotreviewbot

'django.conf.settings' imported but unused

reviewbotreviewbot

'django.test.utils.override_settings' imported but unused

reviewbotreviewbot

The default rate should be a constant.

brenniebrennie

The default rate should be a constant. Additionally, I think this function should be responsible for the default instead of …

brenniebrennie

Care to add doc comments to these?

brenniebrennie

Instead of this, lets make an actual Rate() object.

brenniebrennie

I don't think this should fall back to parsing some default. I think we should raise a ValueError.

brenniebrennie

I'm still not convinced that destructuring this is useful. This is not a tuple.

brenniebrennie

'django.test.utils.override_settings' imported but unused

reviewbotreviewbot

lets call it rate_str, rate since its not a tuple.

brenniebrennie

No blank line here.

brenniebrennie

Leading whitespace.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

'logging' imported but unused

reviewbotreviewbot

'django.test.utils.override_settings' imported but unused

reviewbotreviewbot

local variable 'result' is assigned to but never used

reviewbotreviewbot

undefined name 'assertTrue'

reviewbotreviewbot

djblets_settings should probably never be used. This is really for internal usage for unit tests and such.

chipx86chipx86

Two blank lines here.

chipx86chipx86

I'd call this "login-ratelimit".

chipx86chipx86

You can speed this up by being optimistic about the keys: try: return request.META['HTTP_X_REAL_IP'] except KeyError: try: return request.META['HTTP_X_FORWARDED_FOR'].split(',')[0].strip() except …

chipx86chipx86

This can be one statement.

chipx86chipx86

This can be one statement, which will also be faster: return '%d/%ds%s%s' % (limit, period, user_id_or_ip, _get_window(period))

chipx86chipx86

"Check whether the current ..."

chipx86chipx86

"get_usage_count" is an implementation detail. No need for it in the docs here.

chipx86chipx86

bool, optional

chipx86chipx86

bool, optional

chipx86chipx86

You can fit more per line.

chipx86chipx86

This should be checking django.conf.settings, not djblets.settings. The latter will never be overridden.

chipx86chipx86

I don't like having this in here. This should be solely up to the unit tests.

chipx86chipx86

This shouldn't be necessary. There should always be a suitable default without requiring the consumer to set one, in this …

chipx86chipx86

You can fit more per line.

chipx86chipx86

I want to suggest an alternative version of this that I think will help readability, reduce communication with the cache …

chipx86chipx86

You can collapse this to a elif.

chipx86chipx86

When referencing a function in this class, use: :py:meth:`parse`

chipx86chipx86

This isn't a function.

chipx86chipx86

Blank line here.

chipx86chipx86

This must follow the form of a one-line summary, blank line, multi-line description.

chipx86chipx86

Same as above with the format. Instead of get_usage_count(), do: :py:func:`get_usage_count` (:py:func: is used for top-level functions, :py:meth: for methods …

chipx86chipx86

I don't really know that we need this, given the suggestion above for settings.

chipx86chipx86

ValueError doesn't turn multiple arguments into a string. The user is actually going to see this error message: "('Could not …

chipx86chipx86

The regex already handles ensuring that we have a lowercase value. Given that, You can collapse these into: seconds = …

chipx86chipx86

seconds *= int(multiplier)

chipx86chipx86

Do we really want this? I think callers should be explicit in using rate.count and rate.seconds. Keeps all code self-documenting.

chipx86chipx86

"Return whether ..."

chipx86chipx86

If this is for testing purposes, it should live in the unit tests only.

chipx86chipx86

bool

chipx86chipx86

Can be: return (isinstance(other, Rate) and self.count == other.count and self.seconds == other.seconds)

chipx86chipx86

Alphabetical order.

chipx86chipx86

We have our own TestCase class that should be used instead. See djblets/testing/

chipx86chipx86

Should be more descriptive, so someone looking at these tests can figure out exactly what is being tested. I suggest: …

chipx86chipx86

This should be setUp, not setUpClass, so you have a fresh factory for each test.

chipx86chipx86

This needs to happen in tearDown, not tearDownClass, or each of the unit tests will be sharing the same cache …

chipx86chipx86

You're not testing that Django's settings decorator works. No need for this. Same with ones below.

chipx86chipx86

This can be one statement. You should also have a blank line between the setup and the assertions. Same below.

chipx86chipx86

Blank line between code and new blocks (like with).

chipx86chipx86

This is actually testing if context has a value, and isn't testing the string. The string is not correct either. …

chipx86chipx86

Condense, and blank line before.

chipx86chipx86

testing should never be added to methods.

chipx86chipx86

Why is this change needed?

chipx86chipx86

Should be reverted once testing is removed.

chipx86chipx86

Here, too.

chipx86chipx86

I wouldn't test defaults vs. custom values here. We don't frankly care at this level, so long as the core …

chipx86chipx86

Better to do this in a loop. Reads better.

chipx86chipx86

Should also be in a loop.

chipx86chipx86

Rather than having this here, I'd have the unit tests that depend on this set the value for their tests. …

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
  3. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 36
     E261 at least two spaces before inline comment
    
  4. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 4
     W291 trailing whitespace
    
  6. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  7. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  8. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  9. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  10. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  11. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  12. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  13. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  14. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  15. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 24
     E231 missing whitespace after ','
    
  16. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  17. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  18. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 9
     E116 unexpected indentation (comment)
    
  19. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  20. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  21. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  22. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  23. djblets/auth/ratelimit.py (Diff revision 1)
     
     
    Show all issues
    Col: 68
     W292 no newline at end of file
    
  24. 
      
RK
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 36
     E261 at least two spaces before inline comment
    
  3. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
  4. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 4
     W291 trailing whitespace
    
  6. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  7. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  8. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  9. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  10. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  11. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  12. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  13. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  14. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  15. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 24
     E231 missing whitespace after ','
    
  16. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  17. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 9
     E116 unexpected indentation (comment)
    
  18. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  19. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  20. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  21. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues
    Col: 68
     W292 no newline at end of file
    
  22. 
      
brennie
  1. 
      
  2. djblets/auth/ratelimit.py (Diff revision 2)
     
     
    Show all issues

    By calling the variable prep_cache_key, you are shadowing the function prep_cache_key.

  3. djblets/settings.py (Diff revision 2)
     
     
    Show all issues

    Do we want to give the frequency or the minimum period between attempts? If you use minimum period you can just write this as the value in seconds and you don't have to worry about parsing it.

    e.g. LOGIN_RATE_LIMIT = 20 would be equivalent to 5/m

    Might wanna double check with Christian on this, though.

  4. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/auth/tests/test_ratelimit.py
        djblets/auth/ratelimit.py
        djblets/settings.py
        tests/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/auth/tests/test_ratelimit.py
        djblets/auth/ratelimit.py
        djblets/settings.py
        tests/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 36
     E261 at least two spaces before inline comment
    
  3. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (97 > 79 characters)
    
  4. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
     'cache_memoize' imported but unused
    
  5. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  6. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 4
     W291 trailing whitespace
    
  7. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  9. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 61
     W291 trailing whitespace
    
  10. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  11. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  12. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  13. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  14. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  15. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  16. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  17. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 52
     W291 trailing whitespace
    
  18. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  19. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 61
     W291 trailing whitespace
    
  20. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 60
     W291 trailing whitespace
    
  21. djblets/auth/ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  22. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
     'Group' imported but unused
    
  23. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
     'User' imported but unused
    
  24. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  25. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  26. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  27. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  28. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 48
     E251 unexpected spaces around keyword / parameter equals
    
  29. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  30. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 48
     E251 unexpected spaces around keyword / parameter equals
    
  31. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  32. djblets/auth/tests/test_ratelimit.py (Diff revision 3)
     
     
    Show all issues
    Col: 48
     E251 unexpected spaces around keyword / parameter equals
    
  33. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/auth/tests/test_ratelimit.py
        djblets/auth/ratelimit.py
        djblets/settings.py
        tests/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/auth/tests/test_ratelimit.py
        djblets/auth/ratelimit.py
        djblets/settings.py
        tests/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. 
      
david
  1. These comments are mostly related to documentation style. Looks like a pretty solid start!

  2. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
    Show all issues

    Docstrings should use three double-quotes instead of single. The first line should also be joined with the quotes so the docstring doesn't start with a \n character:

    def user_or_ip(request):
        """Obtain user ID or IP address from request.
    
        If ...
        """
    

    The text should also be full sentences with good grammar (when possible). In this case, probably "Return the user's ID or IP address."

  3. djblets/auth/ratelimit.py (Diff revision 4)
     
     
    Show all issues

    We should list the full type here (django.http.HttpRequest).

  4. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
    Show all issues

    This should include the type:

    Returns:
        unicode:
        ...
    
  5. djblets/auth/ratelimit.py (Diff revision 4)
     
     
    Show all issues

    We should use six.string_type(...) (importing django.utils.six above). On Python 2, str is a bytestring, while on Python 3, it's a unicode string.

  6. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
    Show all issues

    Same here with the docstring. This should also be written in the imperative mood ("Return ..." instead of "Returns ...")

  7. djblets/auth/ratelimit.py (Diff revision 4)
     
     
    Show all issues

    Should be "unicode" instead of "String"

  8. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
    Show all issues

    Should list the return type here.

  9. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
    Show all issues

    Same comment with docstrings.

  10. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
     
     
     
    Show all issues

    Arg types should be unicode. This also needs a Returns.

  11. djblets/auth/ratelimit.py (Diff revision 4)
     
     
     
    Show all issues

    Please add a blank line between each arg.

  12. djblets/auth/ratelimit.py (Diff revision 4)
     
     
    Show all issues

    You don't need the u prefix here because you imported unicode_literals at the top.

  13. djblets/auth/ratelimit.py (Diff revision 4)
     
     
    Show all issues

    Same docstring comments here.

  14. djblets/auth/ratelimit.py (Diff revision 4)
     
     
    Show all issues

    Same docstring comments here.

  15. djblets/auth/tests/test_ratelimit.py (Diff revision 4)
     
     
    Show all issues

    Some of our old test code uses camelCase, but new code should use underscores, such as test_invalid_key

  16. djblets/auth/tests/test_ratelimit.py (Diff revision 4)
     
     
    Show all issues

    Same here.

  17. djblets/auth/tests/test_ratelimit.py (Diff revision 4)
     
     
    Show all issues

    Same here.

  18. djblets/auth/tests/test_ratelimit.py (Diff revision 4)
     
     
    Show all issues

    This can be self.assertFalse(result)

  19. djblets/auth/tests/test_ratelimit.py (Diff revision 4)
     
     
    Show all issues

    This can be self.assertTrue(result)

  20. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/webapi/auth/backends/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (95 > 79 characters)
    
  3. djblets/webapi/auth/backends/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. djblets/webapi/auth/backends/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  5. djblets/webapi/auth/backends/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  6. Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  7. Show all issues
    Col: 64
     W291 trailing whitespace
    
  8. Show all issues
    Col: 80
     E501 line too long (92 > 79 characters)
    
  9. Show all issues
    Col: 44
     E251 unexpected spaces around keyword / parameter equals
    
  10. Show all issues
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  11. Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  12. Show all issues
    Col: 44
     E251 unexpected spaces around keyword / parameter equals
    
  13. Show all issues
    Col: 46
     E251 unexpected spaces around keyword / parameter equals
    
  14. Show all issues
    Col: 80
     E501 line too long (96 > 79 characters)
    
  15. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/settings.py (Diff revision 6)
     
     
    Show all issues
     'settings_local' imported but unused
    
  3. djblets/settings.py (Diff revision 6)
     
     
    Show all issues
     'from settings_local import *' used; unable to detect undefined names
    
  4. djblets/webapi/auth/backends/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 64
     W291 trailing whitespace
    
  5. Show all issues
    Col: 45
     W291 trailing whitespace
    
  6. settings_local.py (Diff revision 6)
     
     
    Show all issues
     'os' imported but unused
    
  7. 
      
RK
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/settings.py (Diff revision 7)
     
     
    Show all issues
     'settings_local' imported but unused
    
  3. djblets/settings.py (Diff revision 7)
     
     
    Show all issues
     'LOGIN_LIMIT_RATE' imported but unused
    
  4. djblets/settings.py (Diff revision 7)
     
     
    Show all issues
     undefined name 'dependency_error'
    
  5. 
      
RK
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/settings.py (Diff revision 8)
     
     
    Show all issues
     'settings_local' imported but unused
    
  3. djblets/settings.py (Diff revision 8)
     
     
    Show all issues
     'LOGIN_LIMIT_RATE' imported but unused
    
  4. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/settings.py (Diff revision 9)
     
     
    Show all issues
     'settings_local' imported but unused
    
  3. djblets/settings.py (Diff revision 9)
     
     
    Show all issues
     'LOGIN_LIMIT_RATE' imported but unused
    
  4. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/settings.py (Diff revision 10)
     
     
    Show all issues
     'settings_local' imported but unused
    
  3. djblets/settings.py (Diff revision 10)
     
     
    Show all issues
     'LOGIN_LIMIT_RATE' imported but unused
    
  4. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        settings_local.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/settings.py (Diff revision 11)
     
     
    Show all issues
     'settings_local' imported but unused
    
  3. djblets/settings.py (Diff revision 11)
     
     
    Show all issues
     'LOGIN_LIMIT_RATE' imported but unused
    
  4. 
      
david
  1. 
      
  2. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Blank line between these (this is in the djblets codebase, so djblets imports are "this module")

  3. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
     
    Show all issues

    Since these are "constants", they should be ALL_CAPS

  4. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Please use single-quotes for strings.

  5. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    This regex string should be prefixed by r (r'([...) to indicate that it's a "raw" string, so the \s behave like we want.

  6. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Dedent this 4 spaces (we don't indent the definition because there's only ever one return value, unlike args where there are many).

  7. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Combine these so the docstring doesn't start with a blank line ("""Return number of ...)

  8. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
     
     
     
    Show all issues

    This should list a single return value, "tuple of int". The description for that return value should say that it's a 2-tuple of (count, seconds) and explain what count and seconds are.

  9. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    If this fails to match, we should print a warning and return a default value.

  10. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Merge these lines.

  11. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Should be "unicode" instead of "String"

  12. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Dedent this 4 spaces.

  13. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Docstrings should use 3 double-quotes. Also, please merge these lines.

  14. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    String -> unicode.

  15. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    String -> unicode.

  16. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
     
    Show all issues

    Should be:

    Returns:
        bool:
        Whether the current user has exceeded ...
    
  17. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Leftover debug code?

  18. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Three double-quotes, and the first line of the docstring should be a single-line summary on the same line as the quotes.

  19. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    String -> unicode.

  20. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    String -> unicode.

  21. djblets/auth/ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Returns:
    dict:
    A dictionary containing ...

  22. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Leftover debug code?

  23. djblets/auth/ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Let's break this out into multiple lines:

    return {
        'count': count,
        'limit': limit,
        'time_left': time_left,
    }
    
  24. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
     
    Show all issues

    Blank line between these.

  25. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Let's create the requestFactory inside the test-case class, probably in a setUpClass method. That way it won't be done unless we're actually executing the test cases (right now it's done when the file is parsed)

  26. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
    Show all issues

    For the unit tests in this file, I think we can merge them all into one test class.

  27. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
    Show all issues

    This docstring should go within the function instead of above it (right now it's acting as the docstring for the class).

    Test docstrings should be of the format "Testing X" to match the others in the suite output.

  28. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Test docstrings should be of the format "Testing X" to match the others in the suite output.

  29. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Test docstrings should be of the format "Testing X" to match the others in the suite output.

  30. djblets/auth/tests/test_ratelimit.py (Diff revision 9)
     
     
    Show all issues

    Test docstrings should be of the format "Testing X" to match the others in the suite output.

  31. djblets/settings.py (Diff revision 9)
     
     
     
     
     
     
    Show all issues

    We shouldn't need/want this. settings_local is something we do in Review Board, but the djblets settings.py file is primarily there to run the unit tests, and isn't used by consumers as anything other than a reference.

  32. djblets/webapi/auth/backends/base.py (Diff revision 9)
     
     
     
    Show all issues

    Blank line between these two.

  33. djblets/webapi/auth/backends/base.py (Diff revision 9)
     
     
     
    Show all issues

    If I'm reading the code correctly, this will update the rate limit info in the cache, even if the authentication is sucessful. That means that we'll be inadvertantly rate-limiting all API requests, even if the credentials are fine.

    I think what we want to do is first check whether the current request is rate-limited and fail if so, but not actually store anything. We can then store iff authentication fails.

  34. djblets/webapi/auth/backends/base.py (Diff revision 9)
     
     
     
    Show all issues

    Leftover debug code?

  35. djblets/webapi/auth/backends/base.py (Diff revision 9)
     
     
    Show all issues

    "ck_ratelimit" is kind of a weird variable name. If you get rid of the debug output, we can just move the check inline:

    if is_ratelimited(...):
        ...
    
  36. Show all issues

    This can wrap a little better to fit within the 80 columns. The second line should also align with the start of the quotes rather than the start of the text.

  37. Show all issues

    Let's rewrap this for better formatting:

    self.assertEqual(
        result,
        (False, 'Maximum number of login attempts exceeded', None))
    
  38. Show all issues

    Let's rewrap this for better formatting:

    self.assertEqual(
        result,
        (False, 'Maximum number of login attempts exceeded', None))
    
  39. 
      
brennie
  1. 
      
  2. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    Summary should be on first line.

  3. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
     
     
     
     
    Show all issues

    We don't have return annotation support for multiple returns or namned returns. This technically returns a tuple.

    However, we can create a new type with namedtuple (from collections). This allows us to create a new tuple class that has named fields that can be accessed with either indices or the field names as attributes. e.g.,

    from collections import namedtuple
    
    
    #: Type summary.
    #:
    #: Type description.
    #:
    #: Attributes:
    #:     count (int):
    #:         ...
    #:
    #:     seconds (int):
    #:         ...
    RateType = namedtuple('RateType', ('count', 'seconds'))
    

    Then we can annotate this return type like so:

    def split_rate(rate)
        """...
    
        Returns:
            RateType:
            ....
        """
        return RateType(count=count, seconds=seconds)
    

    RateType is maybe a bad name. Perhaps Rate? I'll leave that up to you.

  4. djblets/auth/ratelimit.py (Diff revision 11)
     
     
    Show all issues

    What happens if i set RATE_LIMIT = 'nope'? We should raise ImproperlyConfigured if we cannot parse this value.

  5. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    Docstring.

  6. djblets/auth/ratelimit.py (Diff revision 11)
     
     
    Show all issues

    You will not want to use naive times (i.e., without timezones). You will want to do:

    from django.utils import timezone
    
    ts = timezone.now()
    
  7. djblets/auth/ratelimit.py (Diff revision 11)
     
     
    Show all issues

    Does w stand for window? If so, can we name it that?

  8. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    Same comments re: summaries.

  9. djblets/auth/ratelimit.py (Diff revision 11)
     
     
    Show all issues

    unicode.

  10. djblets/auth/ratelimit.py (Diff revision 11)
     
     
    Show all issues

    This shouldn't be indented.

  11. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    Double quotes for docstrings. Summary should be on this line.

    Also, docstrings should be in the imperitive mood, i.e. they should be a command. So this should read "Return ...".

  12. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    Double quotes for docstrings. Summary should be on this line.

    Also, docstrings should be in the imperitive mood, i.e. they should be a command. SO this should read "Return ..."

    Summaries should only be one line.

  13. djblets/auth/ratelimit.py (Diff revision 11)
     
     
     
     
     
    Show all issues

    This is missing the type annotation (dict). You can also use restructuredText to annotate the keys and values.

    dict:
    A dictionary with the following keys:
    
    ``'count'`` (:py:class:`int`):
        ...
    
    ``'limit'`` (:py:class:`int`):
        ...
    
    ``'time_left'`` (:py:class:`int`):
        ...
    

    The above will render as a definition list with links to the appropriate types.

  14. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    We won't want this to be a global. Instead we can put this inside the setUpClass method of RateLimitTests:

    class RateLimitTests(TestCase):
        @classmethod
        def setUpClass(cls):
            super(RateLimitTests, cls).setUpClass()
    
            cls.request_factory = RequestFactory()
    

    Then it will be available as self.request_factory inside the test methods.

  15. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
     
     
     
     
     
     
    Show all issues

    Instead of this, you can use django.contrib.auth.User and django.contrib.auth.AnonymousUser. The latter will always have is_authenticated() return False.

  16. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    There should be a testcase that tests that improper data raises an exception.

  17. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    Blank line between these. Missing period. Missing docstring for test method.

  18. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    This should use self.assertEqual

    It should also probably include a message about the rate being tested. e.g.

    self.assertEqual(count_seconds, split_rate(rate), "split_rate(%r) != %r" % (rate, count_seconds))
    
  19. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    RateLimitTests

    We make our test case class names plural

  20. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    Missing period.

  21. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
     
    Show all issues

    This must call super(RateLimitTests, self).setUp()

    Also, this will not clear the cache after the last test is run so add:

    @classmethod
    def tearDownClass(cls):
        super(RateLimitTests, cls).tearDownClass()
        cache.clear()
    
  22. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    test case names should indicate what feature they are testing. e.g. "Testing rate limit invalid keys" or similar.

    Also it should begin with "Testing"

  23. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    Testing

  24. djblets/auth/tests/test_ratelimit.py (Diff revision 11)
     
     
    Show all issues

    Testing

  25. djblets/settings.py (Diff revision 11)
     
     
     
     
     
     
     
    Show all issues

    settings_local is a Review Board thing. Undo this change.

  26. djblets/webapi/auth/backends/base.py (Diff revision 11)
     
     
    Show all issues

    Single quotes.

  27. Show all issues

    No hanging indent here. It should be flush with the first "

  28. settings_local.py (Diff revision 11)
     
     
    Show all issues

    This file should not be committed.

  29. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 12)
     
     
    Show all issues
    Col: 63
     E225 missing whitespace around operator
    
  3. djblets/auth/ratelimit.py (Diff revision 12)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. djblets/auth/tests/test_ratelimit.py (Diff revision 12)
     
     
    Show all issues
    Col: 80
     E501 line too long (109 > 79 characters)
    
  5. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 13)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. 
      
RK
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. djblets/auth/ratelimit.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. djblets/auth/tests/test_ratelimit.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (86 > 79 characters)
    
  5. djblets/auth/tests/test_ratelimit.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (87 > 79 characters)
    
  6. djblets/auth/tests/test_ratelimit.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  7. djblets/auth/tests/test_ratelimit.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (85 > 79 characters)
    
  8. djblets/webapi/auth/backends/base.py (Diff revision 15)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  9. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. djblets/auth/tests/test_ratelimit.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. djblets/auth/tests/test_ratelimit.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. djblets/auth/tests/test_ratelimit.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  6. djblets/auth/tests/test_ratelimit.py (Diff revision 16)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  7. 
      
RK
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/ratelimit.py (Diff revision 17)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. djblets/auth/tests/test_ratelimit.py (Diff revision 17)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  4. djblets/auth/tests/test_ratelimit.py (Diff revision 17)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  5. djblets/auth/tests/test_ratelimit.py (Diff revision 17)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  6. djblets/auth/tests/test_ratelimit.py (Diff revision 17)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  7. 
      
RK
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        djblets/webapi/auth/backends/base.py
        tests/settings.py
        djblets/auth/tests/test_ratelimit.py
        djblets/webapi/tests/test_api_auth_backend.py
        djblets/auth/ratelimit.py
        djblets/settings.py
    
    Ignored Files:
        djblets/auth/tests/__init__.py
    
    
  2. djblets/auth/tests/test_ratelimit.py (Diff revision 18)
     
     
    Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  3. djblets/auth/tests/test_ratelimit.py (Diff revision 18)
     
     
    Show all issues
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  4. djblets/auth/tests/test_ratelimit.py (Diff revision 18)