You have several lines that contain whitespace. You can set up Sublime to trim this on save via Preferences -> …

Please wrap your description at 72 lines. You can install AutoWrap (Cmd+Shift+P, PCI (for package control install), AutoWrap) and set …

Your description mentions RB but this is a change to Djblets. Your future change to RB should refer to those …

Your summary should be in the imperitive mood, i.e., it should read like you are giving a command or order. …

Your summary should be in the imperitive mood, i.e., it should read like a command. For example: Update querystring_with to …

Can you add unit tests for {% querystring_with_fragments "foo" %}

Can you add unit tests for {% querystring_with_fragments "foo=bar=baz" %}

Can you add unit tests for Can you add unit tests for {% querystring_with_fragments "foo=" %}

Can you add unit tests for {% querystring_with_fragments "foo bar=baz qux" %}

Can you add unit tests for {% querystring_with_fragments "=foo" %}

Can you add unit tests for {% querystring_with_fragments "a=1" "a=2" "a=3 %}? The result should be ?a=1&a=2&a=3.

Can you add unit tests for {% querystring_with_fragments "a=b=c=d" "e=f=g=h" %}

Your summary should be in the imperitive mood (i.e., it should read like a command or order). How about: Add …

Can you add unit tests with unicode keys and values with e.g. han characters?

Description has a grammar-o: "There are three different modes are"

Can you add a test for {% querystring "remove" "foo=1" "foo=2" %} for a querystring ?foo=1&foo=2&foo=3?

W291 trailing whitespace

This documentation needs to be updated to indicate that the function can update multiple query parameters.

This function no longer takes these aruments. You will want to update this section.

I did some talking with Christian and I'm not convinced this is the best way to handle this. Instead, we …

E226 missing whitespace around arithmetic operator

Since attr and value are used immediately, we can move them inline, like so: query[args[i].encode('utf-8')] = args[i + 1].encode('utf-8')

E226 missing whitespace around arithmetic operator

W293 blank line contains whitespace

Undo this change.

W293 blank line contains whitespace

W293 blank line contains whitespace

tmp isn't a very descriptive variable name. How about render_result?

E303 too many blank lines (2)

Can you add a trailing comma?

E126 continuation line over-indented for hanging indent

This is only used once so it could be moved inline.

t_dict isn't a very expressive variable name. How about expected_result? However, it can be moved inline (see next comment)

I think it would be fine to do: self.assertEqual( parse_qs(render_result[1:]), { 'bar': ['baz'], 'foo': ['bar'], })

E303 too many blank lines (2)

W293 blank line contains whitespace

Can you add a trailing comma? This should also be dedented so that it looks like: render_result = t.render(Context({ 'request': …

See comments on previous function about moving things inline.

W391 blank line at end of file

E501 line too long (91 > 79 characters)

E501 line too long (91 > 79 characters)

E501 line too long (91 > 79 characters)

We are going to want to make this into a new template tag. The reason being is that an old …

Docstring summary should fit on a single line.

Missing context (which is a django.template.Context)

*args (tuple)

Documentation should always be sentence-case. How about: Multiple querystring fragments (e.g., "foo=1") that will be used to update the initial …

These should no longer be here.

These tests would be easier to read if the initial state was: { 'foo': 'foo', 'bar': 'bar', 'baz': 'baz', 'qux': …

Typo: `context.

While technically correct, how about: The Django template rendering context. It is what we use elsewhere.

Blank line between these.

Undo this indentation.

""" on next line.

context

See comment on other templatetag about this.

The description should not be indented, i.e. unicode: The new URL ...

Wrong templatetag :)

This should use the parse_qs method becuase dict iteration order is not guaranteed.

Mind doing bar: "bar" just to keep in line with other examples?

Mind ordering this foo then bar like above?

E501 line too long (88 > 79 characters)

E501 line too long (90 > 79 characters)

E501 line too long (88 > 79 characters)

E128 continuation line under-indented for visual indent

E501 line too long (81 > 79 characters)

E303 too many blank lines (2)

F401 'django.utils.six.moves.urllib.parse.parse_qs' imported but unused

E501 line too long (85 > 79 characters)

E231 missing whitespace after ','

E501 line too long (83 > 79 characters)

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E128 continuation line under-indented for visual indent

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E122 continuation line missing indentation or outdented

E303 too many blank lines (2)

E127 continuation line over-indented for visual indent

W391 blank line at end of file

E303 too many blank lines (2)

E501 line too long (85 > 79 characters)

E501 line too long (83 > 79 characters)

six.moves before six.moves.urllib

""" on next line.

Undo

This should be a header of Args and describe what it is in addition to its values. E.g. Args: mode …

This just returns the querystring, not the URL.

The < should line up with the c in code-block.

Instead of having this highly nested, we can pull things out into temporaries to make it more readable. How about: …

Can we add support to remove specific key-vaue pairs, in addition to removing the entire set? e.g. "remove" "a=4" would …

How about: query.pop(arg.encode('utf-8'), None) This does not require the try/except.

You can use the same approach I outlined above to make this more readable.

The tag is no longer called QuerystringWithFragments

The < in <a should be aligned with the c in the code-block above.

This message is no longer correct. It should include the update mode.

Can we format like the following example? This makes it clear it is a string literal and renders as a …

This should be first in the list of args. This is missing its type.

These all need to be de-dented one space so that the first character lines up with the c in code-block. …

No blank line here.

The implementation of QueryDict.urlencode actually forces the text to bytes, so we don't need to encode the attrs or values …

No blank line here.

No blank line here.

No blank line here.

This is no longer required.

Six has moves support for this: from django.utils.six.moves.html_parser import HTMLParser

No blank line after function docstrings. Here and below.

This one doesn't assert that rendered_result.startswith('?').

""" on next line. "args get" fits on previous line.

This blank line is unnecessary here and in all tests below.

No period at the end of test docstrings (because they print out as <DOCSTRING> ... OK). Here and below.

This fits on a single line. You seem to be wrapping your docstrings at ~70 rather than 79. Please correct …

E501 line too long (116 > 79 characters)

Undo this change, please.

Undo this change, please.

This is not indented a multiple of four. (It is missing one space.)

Should not be indented relative to prior line.

Remove this blank line

This is setting the same list in a loopp len(parsed.getlist(attr)) times. This also doesn't need to encode values. What this …

Remove this line.

Blank line between these.

Remove this line.

I think this will miss some cases, e.g. {% querystring "remove" "x&y=1&z=" %}. Now, this is an edge case, but …

Again, no need to encode.

We only need to do getlist once since we are modifying the same list in each iteration of the loop

Remove this line.

Since we are appending every single entry in args into query, we can simply do: for arg in args: query.update(QueryDict(arg)) …

Again, no need to encode.

Again, no need to encode.

Can you surround "foo=1" with double backticks, e.g. ``"foo=1"``

Can you fix the formatting here? These can both go on the same line.

The second for loop needs to be nested in the first, otherwise only the last element in args will be …

This needs to be indented to be aligned with the if. It is currently always executing because there is no …

This will actually be a django.template.RequestContext.

This should not be indented.

No blank line here.

The key=value should be in quotes, right? For "mode", is the equivalent always "update"? If so, we can say that …

Too long for the line. Must fit in 79 characters. It's also missing an ending period.

See the type above.

This can be on the same line.

This is going to set the list of values for every value in the list of values. You should be …

This doesn't seem right. We're overriding to_remove every time, meaning we're only ever getting the list for the last attribute. …

This should raise a TemplateSyntaxError.

This is in the wrong import group.

The trailing period should remain. You only remove this for docstrings for unit tests themselves (as they're outputted to the …

The tests in this module should be updated to check for warnings as well. Search the codebase for catch_warnings for …

Same here.

Needs a trailing period.

This should be removed.

Last entries in a dictionary should always have a trailing comma.

I'd recommend just setting self.request in setUp() (not setUpClass(), since it'll be modified) and referencing it in all the tests.

Trailing comma.

""" on the next line. "overridden".

Trailing comma.

Best to explicitly check the resulting value, rather than introducing another parsing/checking step. That just leads to problems, as more …

"overridden" I'd also add "that" before "get".

You can start the template on the first line, like: t = Template('{% load djblets_utils %}' '{% querystring .... %}') …

Trailing comma.

Trailing comma.

Trailing comma.

Trailing comma.

""" on the next line. "overridden"

Test docstrings that go over one line need to have """ on the following line. Same for all other tests …

Trailing comma.

""" on the next line. No trailing period.

This should be removed.

"existing"

""" on the next line.

Trailing comma.

""" on the next line.

Trailing comma.

""" on the next line. "a key fragments" doesn't make sense. Maybe just "a key fragment?" What's a key fragment?

Trailing comma.

""" on the next line.

Trailing comma.

This should be removed.

""" on the next line.

Trailing comma.

"non-existing" """ on the next line.

Trailing comma.

""" on the next line.

Trailing comma.

""" on the next line.

Trailing comma.

""" on the next line.

Trailing comma.

""" on the next line.

Trailing comma.

""" on the next line.

Trailing comma.

The } should line up with the % and the D in DeprecationWarning.

Can you specify that this comes from this template tag, e.g. raise TemplateSyntaxError('Invalid mode for {%% querystring %%}: %s' % …

Single quotes here.