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

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

Col: 36 E261 at least two spaces before inline comment

Col: 1 E302 expected 2 blank lines, found 1

Col: 4 W291 trailing whitespace

Col: 1 W293 blank line contains whitespace

Col: 1 E302 expected 2 blank lines, found 1

Col: 1 W293 blank line contains whitespace

Col: 1 W293 blank line contains whitespace

Col: 1 W293 blank line contains whitespace

Col: 1 W293 blank line contains whitespace

Col: 1 W293 blank line contains whitespace

Col: 1 E302 expected 2 blank lines, found 1

Col: 1 E302 expected 2 blank lines, found 1

Col: 1 E302 expected 2 blank lines, found 1

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

Col: 1 W293 blank line contains whitespace

Col: 68 W292 no newline at end of file

Col: 1 E302 expected 2 blank lines, found 1

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

'cache_memoize' imported but unused

Col: 61 W291 trailing whitespace

Col: 52 W291 trailing whitespace

Col: 1 E302 expected 2 blank lines, found 1

Col: 61 W291 trailing whitespace

Col: 60 W291 trailing whitespace

'Group' imported but unused

'User' imported but unused

Col: 1 E302 expected 2 blank lines, found 1

Col: 1 E302 expected 2 blank lines, found 1

Col: 1 E302 expected 2 blank lines, found 1

Col: 46 E251 unexpected spaces around keyword / parameter equals

Col: 48 E251 unexpected spaces around keyword / parameter equals

Col: 46 E251 unexpected spaces around keyword / parameter equals

Col: 48 E251 unexpected spaces around keyword / parameter equals

Col: 46 E251 unexpected spaces around keyword / parameter equals

Col: 48 E251 unexpected spaces around keyword / parameter equals

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

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

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

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 …

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

Should be "unicode" instead of "String"

Should list the return type here.

Same comment with docstrings.

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

Please add a blank line between each arg.

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

Same docstring comments here.

Same docstring comments here.

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

Same here.

Same here.

This can be self.assertFalse(result)

This can be self.assertTrue(result)

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

Col: 1 W293 blank line contains whitespace

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

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

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

Col: 64 W291 trailing whitespace

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

Col: 44 E251 unexpected spaces around keyword / parameter equals

Col: 46 E251 unexpected spaces around keyword / parameter equals

Col: 9 E303 too many blank lines (2)

Col: 44 E251 unexpected spaces around keyword / parameter equals

Col: 46 E251 unexpected spaces around keyword / parameter equals

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

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

Col: 64 W291 trailing whitespace

Col: 45 W291 trailing whitespace

'os' imported but unused

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

Since these are "constants", they should be ALL_CAPS

Please use single-quotes for strings.

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

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

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

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

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

Merge these lines.

Should be "unicode" instead of "String"

Dedent this 4 spaces.

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

String -> unicode.

String -> unicode.

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

Leftover debug code?

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

String -> unicode.

String -> unicode.

Returns: dict: A dictionary containing ...

Leftover debug code?

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

Blank line between these.

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

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

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

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

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

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

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

Blank line between these two.

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

Leftover debug code?

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

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

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

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

Summary should be on first line.

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

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

Docstring.

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

Same comments re: summaries.

unicode.

This shouldn't be indented.

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

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

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

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

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

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

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

RateLimitTests We make our test case class names plural

Missing period.

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

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

Testing

Testing

settings_local is a Review Board thing. Undo this change.

Single quotes.

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

This file should not be committed.

Col: 63 E225 missing whitespace around operator

Col: 1 W293 blank line contains whitespace

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

Col: 1 E302 expected 2 blank lines, found 1

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Bare imports should be above from ...

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

No need for [] around \d

How about get_user_id_or_ip?

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

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

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

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

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

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

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

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

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

What exceptions are we expecting to hit?

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

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

This can be private.

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

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

user_id_or_ip is a better variable.

Blank line between these.

int, not Integer. Same elsewhere.

This line should only have the type (int).

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

This can be private.

Same comment as above with the naming.

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

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

Missing docs for increment.

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

Blank line here.

No blank line here.

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

Parameters should come after get_usage_count like other functions.

Must be one line.

This is missing increment.

Blank line here.

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

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

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

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

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

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

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

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

'namedtuple' imported but unused

Col: 8 W291 trailing whitespace

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

Col: 1 W293 blank line contains whitespace

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

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

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

Col: 73 W291 trailing whitespace

'zlib' imported but unused

'ImproperlyConfigured' imported but unused

Col: 68 W291 trailing whitespace

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

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

'User' imported but unused

'ImproperlyConfigured' imported but unused

'override_settings' imported but unused

'DEFAULT_KEY' imported but unused

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

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

Missing a period.

This information belongs in the Returns section.

typo: "THe"

No comma here.

Delete this line.

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

Args does not match up with the actual argument list.

This can all be one line.

This can be one line.

Single line.

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

Missing period.

Expand what ck stands for, please.

Add a trailing comma.

Missing a docstring.

This should be a constant on the class.

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

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

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

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

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

Missing docstring.

Missing docstring.

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

This isn't used.

We generally use the format: Testing Rate.parse

This can be a single statement.

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

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

This can be a singel statement.

Same here re: docstring.

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

This can be a single statement.

This seems to be repeated from above.

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

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

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

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

Col: 1 W293 blank line contains whitespace

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

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

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

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

Unnecessary.

e.g. over ex. Missing period.

Variables usually should have noun names. How about prepped_cache_key?

Technically, is False :) Missing a period.

This comment is unnecessary.

Can we call this initial_value ?

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

Blank line between these.

If multi is multiplier can we rename it to that?

Same comment here re: docstrings. Also missing args.

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

Again, I don't think this is appropriate.

Blank line between these.

Col: 67 W291 trailing whitespace

Col: 1 W293 blank line contains whitespace

Col: 1 W293 blank line contains whitespace

I think this would be better on one line.

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

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

Same as above.

Should this be at line 167?

There's an extra newline I think.

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

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

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

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

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

And here.

And here.

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

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

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

'zlib' imported but unused

"Utilities"

Can you document this with a #: doc comment ?

Reflow this.

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

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

wouldnt this be rate_limit ?

Missing period.

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

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

Missing file docstring.

This docstring doesnt fit.

This doesn't need to be inside the with.

The default rate should be a constant.

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

Care to add doc comments to these?

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

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

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

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

No blank line here.

Leading whitespace.

No blank line here.

No blank line here.

'logging' imported but unused

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

undefined name 'assertTrue'

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

Two blank lines here.

I'd call this "login-ratelimit".

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 …

This can be one statement.

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

"Check whether the current ..."

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

bool, optional

bool, optional

You can fit more per line.

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

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

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

You can fit more per line.

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

You can collapse this to a elif.

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

This isn't a function.

Blank line here.

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

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 …

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

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

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

seconds *= int(multiplier)

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

"Return whether ..."

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

bool

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

Alphabetical order.

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

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

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

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

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

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

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

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

Condense, and blank line before.

testing should never be added to methods.

Why is this change needed?

Should be reverted once testing is removed.

Here, too.

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

Better to do this in a loop. Reads better.

Should also be in a loop.

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