Add {% querystring %} template tag to add, remove, and update querystring parameters

Review Request #9712 — Created March 1, 2018 and submitted

Information

Djblets
release-1.0.x
22f8586...

Reviewers

Previously, it was only possible to update a single query parameter with
{% querystring_with %}. However, in the name of future proofing, it
became necessary to make a new template tag {% querystring %}
that can take an arbitrary number of key-value pairs now with different
modes in the form of:

{% querystring “mode” "key1=value1" "key2=value2" %}

The three different modes are:
“remove” - Will remove keys from the query string.
“append” - Will append values for its given key without overwriting.
“update” - Will add to or replace part of a query string.

Ran unit tests.

Description From Last Updated

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

W291 trailing whitespace

reviewbotreviewbot

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

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

brenniebrennie

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Undo this change.

brenniebrennie

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

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

brenniebrennie

E303 too many blank lines (2)

reviewbotreviewbot

Can you add a trailing comma?

brenniebrennie

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

E303 too many blank lines (2)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

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

brenniebrennie

See comments on previous function about moving things inline.

brenniebrennie

W391 blank line at end of file

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

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

brenniebrennie

Docstring summary should fit on a single line.

brenniebrennie

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

brenniebrennie

*args (tuple)

brenniebrennie

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

brenniebrennie

These should no longer be here.

brenniebrennie

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

brenniebrennie

Typo: `context.

brenniebrennie

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

brenniebrennie

Blank line between these.

brenniebrennie

Undo this indentation.

brenniebrennie

""" on next line.

brenniebrennie

context

brenniebrennie

See comment on other templatetag about this.

brenniebrennie

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

brenniebrennie

Wrong templatetag :)

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Mind ordering this foo then bar like above?

brenniebrennie

E501 line too long (88 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (88 > 79 characters)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (81 > 79 characters)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

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

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

six.moves before six.moves.urllib

brenniebrennie

""" on next line.

brenniebrennie

Undo

brenniebrennie

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

brenniebrennie

This just returns the querystring, not the URL.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

The tag is no longer called QuerystringWithFragments

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

This is no longer required.

brenniebrennie

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

brenniebrennie

No blank line after function docstrings. Here and below.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

E501 line too long (116 > 79 characters)

reviewbotreviewbot

Undo this change, please.

brenniebrennie

Undo this change, please.

brenniebrennie

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

brenniebrennie

Should not be indented relative to prior line.

brenniebrennie

Remove this blank line

brenniebrennie

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

brenniebrennie

Remove this line.

brenniebrennie

Blank line between these.

brenniebrennie

Remove this line.

brenniebrennie

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

brenniebrennie

Again, no need to encode.

brenniebrennie

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

brenniebrennie

Remove this line.

brenniebrennie

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

brenniebrennie

Again, no need to encode.

brenniebrennie

Again, no need to encode.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

This will actually be a django.template.RequestContext.

chipx86chipx86

This should not be indented.

chipx86chipx86

No blank line here.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

See the type above.

chipx86chipx86

This can be on the same line.

chipx86chipx86

I think you can just do: query = context['request'].GET.copy() The copy will be mutable.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This should raise a TemplateSyntaxError.

chipx86chipx86

This is in the wrong import group.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

Needs a trailing period.

chipx86chipx86

This should be removed.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Trailing comma.

chipx86chipx86

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

chipx86chipx86

Trailing comma.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Trailing comma.

chipx86chipx86

Trailing comma.

chipx86chipx86

Trailing comma.

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line. "overridden"

chipx86chipx86

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

brenniebrennie

Trailing comma.

chipx86chipx86

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

chipx86chipx86

This should be removed.

chipx86chipx86

"existing"

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

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

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

This should be removed.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

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

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

""" on the next line.

chipx86chipx86

Trailing comma.

chipx86chipx86

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

brenniebrennie

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

brenniebrennie

Single quotes here.

brenniebrennie
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
  1. Thanks for the patch Mandeep!

    I've left some mostly-stylistic comments here as it looks pretty good.

    However, I think it might be better to take a different approach (which I outline in one of the comments).

    As you fix issues locally, make sure to mark them as fixed here and then, once you've fixed them all, commit and re-post (rbt post -u).

    Thanks again!

  2. Show all issues

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

    Preferences -> Settings -> User and adding the following:

        "trim_trailing_white_space_on_save": true
    

    and

        "ensure_newline_at_eof_on_save": true
    
  3. Show all issues

    Please wrap your description at 72 lines.

    You can install AutoWrap (Cmd+Shift+P, PCI (for package control install), AutoWrap) and set the following settings for the Git Commit syntax

    {
      "rulers": [52, 72],
      "auto_wrap": true,
      "auto_wrap_width": 72
    }
    

    (To edit the syntax setting for a specific filetype you can open a new file and do Cmd+Shift+P, and type syntax git commit. Then go to Preferences -> Settings -> Syntax Specific and add those settings.)

  4. Show all issues

    Your description mentions RB but this is a change to Djblets. Your future change to RB should refer to those changes since Djblets has no notion review requests, etc.

    How about:

    Previously, it was only possible to update a single query parameter with
    `{% querystring_with %}`. However, it is frequently the case that it
    would be handy to update the querystring from within the template rather
    than having to enumerate all possible cases in the view. Now
    `{% querystring_with %}` can take an arbitrary number of key-value pairs
    in the form of:
    
    ```html+django
    {% querystring_with "key1" "value1" "key2" "value2" %}
    ```
    

    Our description and testing done fields support markdown

  5. Show all issues

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

    Update querystring_with to take an arbitrary number of key-value pairs

  6. Show all issues

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

  7. djblets/util/templatetags/djblets_utils.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

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

  8. Show all issues

    I did some talking with Christian and I'm not convinced this is the best way to handle this. Instead, we may want to forgo the simple parsing that register.simple_tag gives us and do our own parsing so that we could do:

    {% querystring_with sorted='1' page='2' foo-bar='baz' %}
    

    I can work with you to write a templatetag parser that understands this syntax.

    1. I honestly think the proper way of going is to pass the equivalent of a query string, instead of creating our own syntax. So instead, something like this:

      {% querystring_with "sorted=1" "page=2" "foo-bar=baz" %}
      

      The reason is that, while query string arguments are often in key=value form, that's just a convention and not a requirement. If we create our own syntax here, it might look nice but it might not be future-proof (for instance, if used with generating URLs for third-party services we're integrating with).

      These are all valid query strings:

      # Pretty standard
      ?sorted=1&page=2
      
      # Values aren't required
      ?sorted=
      ?sorted
      
      # Nor are keys, really
      ?=873613761823
      
      # Technically, ; is equivalent to &
      ?a=b;c=d
      
      # Keys with spaces are allowed, using any variant
      ?a%20b=def
      ?a+b=def
      ?a b=def
      
      # '=' is valid in values
      ?a=b=c=d
      
      # There are different variations on arrays in query strings
      ?a[]=1&a[]=2&a[]=3
      ?a[0]=1&a[1]=2&a[2]=3
      ?a=1&a=2&a=3
      

      By using the argument form provided above, we can represent any of the above without problems:

      {% queryset_with "sorted=1" "page=2" %}
      {% queryset_with "sorted=" %}
      {% queryset_with "sorted" %}
      {% queryset_with "=873613761823" %}
      {% queryset_with "a b=def" %}
      {% queryset_with "a=b=c=d" "e=f=g=h" %}
      {% queryset_with "a=1" "a=2" "a=3" %}
      

      It's only a step away from providing the entire query string with & separations, which we could do, but the advantage of breaking it apart here is readability and the ability to use variables for some of those, like:

      {% if sorted %}
      {%  definevar reverse_sort %}sorted=0{% enddefinevar %}
      {% else %}
      {%  definevar reverse_sort %}sorted=1{% enddefinevar %}
      {% endif %}
      
      ...
      
      {% queryset_with reverse_sort "view=all" %}
      

      Python's parse_qsl handles all of the queryset variations I mentioned (when used with keep_blank_values=True), and Django wraps this with django.http.QueryDict, which knows how to deal with arrays and to re-encode back as a query string. That means we can easily leverage QueryDict to do the hard work for us, basing it on the current page's query string and then updating it with the parsed version, spitting out a new query string we're able to use.

      (Note that we'll want unit tests for all the variations above as well.)

  9. djblets/util/templatetags/djblets_utils.py (Diff revision 1)
     
     
     
     
    Show all issues

    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')
    
  10. Show all issues

    Undo this change.

  11. Show all issues

    tmp isn't a very descriptive variable name.

    How about render_result?

  12. Show all issues

    Can you add a trailing comma?

  13. Show all issues

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

  14. Show all issues

    t_dict isn't a very expressive variable name.

    How about expected_result?

    However, it can be moved inline (see next comment)

  15. Show all issues

    I think it would be fine to do:

    self.assertEqual(
        parse_qs(render_result[1:]),
        {
            'bar': ['baz'],
            'foo': ['bar'],
        })
    
  16. Show all issues

    Can you add a trailing comma?

    This should also be dedented so that it looks like:

    render_result = t.render(Context({
        'request': request,
    })
    
  17. Show all issues

    See comments on previous function about moving things inline.

  18. 
      
mandeep
Review request changed
Commit:
ccf26d2281df2820d0ddd346f6c9cacb8cef50d5
c05d8ff2e0016e92754f72f36d1353367dcad645

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mandeep
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mandeep
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mandeep
brennie
  1. 
      
  2. Show all issues

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

    Update querystring_with to support an arbitrary number of attributes and values
    
  3. djblets/util/templatetags/djblets_utils.py (Diff revision 5)
     
     
     
    Show all issues

    Docstring summary should fit on a single line.

  4. Show all issues

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

  5. Show all issues

    *args (tuple)

  6. djblets/util/templatetags/djblets_utils.py (Diff revision 5)
     
     
     
    Show all issues

    Documentation should always be sentence-case.

    How about:

    Multiple querystring fragments (e.g., "foo=1") that will be used to update the initial querystring.
    
  7. djblets/util/templatetags/djblets_utils.py (Diff revision 5)
     
     
     
     
     
     
    Show all issues

    These should no longer be here.

  8. djblets/util/tests/test_djblets_utils_tags.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    These tests would be easier to read if the initial state was:

    {
    'foo': 'foo',
    'bar': 'bar',
    'baz': 'baz',
    'qux': 'qux',
    }
    

    and the templatetag changed them to something else.

  9. 
      
brennie
  1. 
      
  2. Show all issues

    We are going to want to make this into a new template tag.

    The reason being is that an old usage of {% querystring_with "foo" "bar" %} which used to do ?foo=bar now does ?foo&bar (and both are correct usages).

    So we will want this to do warning.warn("...", DeprecationWarning) with instructions to use the new template tag (lets call it querystring_with_fragments).

  3. 
      
mandeep
mandeep
mandeep
mandeep
brennie
  1. 
      
  2. Show all issues

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

  3. Show all issues

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

  4. Show all issues

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

  5. Show all issues

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

  6. Show all issues

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

  7. Show all issues

    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.

    1. How do you deal with the cases where a is already present in the query string and

      1. we want to add more values of a; or
      2. we want to replace a?
  8. Show all issues

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

  9. Show all issues

    Typo: `context.

  10. djblets/util/templatetags/djblets_utils.py (Diff revision 7)
     
     
     
    Show all issues

    While technically correct, how about:

    The Django template rendering context.
    

    It is what we use elsewhere.

  11. djblets/util/templatetags/djblets_utils.py (Diff revision 7)
     
     
     
    Show all issues

    Blank line between these.

  12. djblets/util/templatetags/djblets_utils.py (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues

    Undo this indentation.

  13. Show all issues

    """ on next line.

  14. Show all issues

    context

  15. Show all issues

    See comment on other templatetag about this.

  16. djblets/util/templatetags/djblets_utils.py (Diff revision 7)
     
     
     
    Show all issues

    The description should not be indented, i.e.

    unicode:
    The new URL ...
    
  17. Show all issues

    Wrong templatetag :)

  18. Show all issues

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

  19. Show all issues

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

  20. Show all issues

    Mind ordering this foo then bar like above?

  21. 
      
mandeep
Review request changed
Summary:
Added a new template tag querystring_with_fragments to support an arbitrary number of attributes and values
Updated template tag querystring_with_fragments to support "modes"
Description:
   

Previously, it was only possible to update a single query parameter with

    {% querystring_with %}. However, it is frequently the case that it
    would be handy to update the querystring from within the template rather
    than having to enumerate all possible cases in the view. Now
    {% querystring_with_fragments %} can take an arbitrary number of
~   key-value pairs in the form of:

  ~ key-value pairs in the form of now with different modes:

   
   

   
~  

{% querystring_with "key1=value1" "key2=value2" %}

  ~

{% querystring_with “mode” "key1=value1" "key2=value2" %}

  +
  +

There are three different modes are:

  + “remove” - Will remove keys from the query string.
  + “append” - Will append values for its given key without overwriting.
  + “update” - Will add to or replace part of a query string.

Testing Done:
~  

Ran unit tests

  ~

Ran unit tests.

Commit:
5887d815320d43e69fc74815b6f405d2df9c0006
a8a77edf85e9aacc716ca899e9ebd728852cdabd

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mandeep
Review request changed
Commit:
a8a77edf85e9aacc716ca899e9ebd728852cdabd
dcbbc35d43ba1c1736f2c8ac9efb054b5119b764

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mandeep
mandeep
brennie
  1. 
      
  2. Show all issues

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

    How about:

    Add {% querystring %} template tag to add, remove, and update querystring parameters
    
  3. Show all issues

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

  4. djblets/util/templatetags/djblets_utils.py (Diff revision 10)
     
     
     
    Show all issues

    six.moves before six.moves.urllib

  5. Show all issues

    """ on next line.

  6. Show all issues

    Undo

  7. djblets/util/templatetags/djblets_utils.py (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

    Args:
    
        mode (unicode):
            How the querystring will be modified. This should be one of the following values:
    
            ``"update"``:
                Replace the values for the specified key(s) in the query string.
    
            ``"append"``:
                Add new values for the specified key(s) to the query string.
    
            ``"remove"``:
                Remove the specified key(s) from the query string.
    
                If no value is provided, all instances of the key will be removed.
    
  8. Show all issues

    This just returns the querystring, not the URL.

  9. djblets/util/templatetags/djblets_utils.py (Diff revision 10)
     
     
     
     
     
    Show all issues

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

  10. djblets/util/templatetags/djblets_utils.py (Diff revision 10)
     
     
     
     
     
     
    Show all issues

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

    How about:

    for arg in args:
        parsed = QueryDict(arg)
    
        for key in part:
            key = key.encode('utf-8')
            values = [
                value.encode('utf-8')
                for value in sparsed.getlist(key)
            ]
            query.setlist(key, value)
    
  11. Show all issues

    Can we add support to remove specific key-vaue pairs, in addition to removing the entire set?

    e.g.

    "remove" "a=4" would remove "a=4" from ?a=1&a=2&a=3&a=4 leaving a=1&a=2&a=3.

    1. I can make a test for this and confirm

  12. djblets/util/templatetags/djblets_utils.py (Diff revision 10)
     
     
     
     
     
    Show all issues

    How about:

    query.pop(arg.encode('utf-8'), None)
    

    This does not require the try/except.

  13. Show all issues

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

  14. Show all issues

    The tag is no longer called QuerystringWithFragments

  15. 
      
mandeep
mandeep
brennie
  1. Some minor style nitpicks. Otherwise this looks good to me.

  2. Show all issues

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

  3. Show all issues

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

  4. djblets/util/templatetags/djblets_utils.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can we format like the following example? This makes it clear it is a string literal and renders as a <code> element in the docs.

    ``'update'``:
        ....
    
    ``'append'``:
        ....
    
    ``'remove'``:
        ....
    
  5. Show all issues

    This should be first in the list of args.

    This is missing its type.

  6. djblets/util/templatetags/djblets_utils.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

    reStructuredText code blocks are only indented three spaces instead of the usual four that we use.

    You may want to enable "show indents" in ST3 (View->Indentation).

  7. Show all issues

    No blank line here.

  8. Show all issues

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

    See: implementation

  9. Show all issues

    No blank line here.

  10. Show all issues

    No blank line here.

  11. Show all issues

    No blank line here.

  12. Show all issues

    This is no longer required.

  13. djblets/util/tests/test_djblets_utils_tags.py (Diff revision 11)
     
     
     
     
     
     
     
    Show all issues

    Six has moves support for this:

    from django.utils.six.moves.html_parser import HTMLParser
    
  14. Show all issues

    No blank line after function docstrings. Here and below.

  15. Show all issues

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

  16. Show all issues

    """ on next line.

    "args get" fits on previous line.

  17. Show all issues

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

  18. Show all issues

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

  19. djblets/util/tests/test_djblets_utils_tags.py (Diff revision 11)
     
     
     
    Show all issues

    This fits on a single line.

    You seem to be wrapping your docstrings at ~70 rather than 79. Please correct this here and below.

  20. 
      
mandeep
Review request changed
Commit:
bc0f79dcfb6e715c1efc673e636e3d4b4cdbeb83
b4f86bfa7e642d94dcebb52c2c11daa12d55a585

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

mandeep
brennie
  1. Again, mostly some formatting nits. I did have a few suggestions on ways to simplify a few things :)

  2. Show all issues

    Description has a grammar-o:

    "There are three different modes are"

  3. djblets/util/templatetags/djblets_utils.py (Diff revision 13)
     
     
     
     
    Show all issues

    Undo this change, please.

  4. djblets/util/templatetags/djblets_utils.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
    Show all issues

    Undo this change, please.

  5. djblets/util/templatetags/djblets_utils.py (Diff revision 13)
     
     
     
    Show all issues

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

  6. Show all issues

    Should not be indented relative to prior line.

  7. Show all issues

    Remove this blank line

  8. djblets/util/templatetags/djblets_utils.py (Diff revision 13)
     
     
     
     
     
     
     
    Show all issues

    This is setting the same list in a loopp len(parsed.getlist(attr)) times.

    This also doesn't need to encode values.

    What this should be is:

    query.setlist(parsed.getlist(attr))
    
  9. Show all issues

    Remove this line.

  10. djblets/util/templatetags/djblets_utils.py (Diff revision 13)
     
     
     
    Show all issues

    Blank line between these.

  11. Show all issues

    Remove this line.

  12. Show all issues

    Again, no need to encode.

  13. Show all issues

    Remove this line.

  14. djblets/util/templatetags/djblets_utils.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
    Show all issues

    Since we are appending every single entry in args into query, we can simply do:

    for arg in args:
        query.update(QueryDict(arg))
    

    QueryDict.update appends to lists instead of overwriting them.

  15. Show all issues

    Again, no need to encode.

  16. Show all issues

    Again, no need to encode.

  17. 
      
mandeep
mandeep
brennie
  1. 
      
  2. djblets/util/templatetags/djblets_utils.py (Diff revisions 13 - 14)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

    We know that in the above example it will result in:

    {'x': [''], 'y': ['1'], 'z': ['']}
    

    We can't really distinguish between z= and z in the argument, so we should treat them both the same.

    for attr in parsed:
        values = parsed.getlist(attr)
    
        if values == ['']:
            # An empty value means either `attr` or `attr=` was provided.
            # In either case, remove all values.
            query.pop(attr, None)
        else:
            values = query.getlist(attr)
    
            for value in parsed.getlist(attr):
                try:
                    values.remove(value)
                except ValueError:
                    pass
    
  3. djblets/util/templatetags/djblets_utils.py (Diff revisions 13 - 14)
     
     
    Show all issues

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

  4. 
      
mandeep
brennie
  1. 
      
  2. Show all issues

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

    ``"foo=1"``
    
  3. djblets/util/templatetags/djblets_utils.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
    Show all issues

    The second for loop needs to be nested in the first, otherwise only the last element in args will be removed form the querystring. Can you add a unit test that does multiple removes? e.g. {% querystring "remove" "a" "b" "c=1" %}

  4. djblets/util/templatetags/djblets_utils.py (Diff revision 15)
     
     
     
     
     
     
     
     
     
    Show all issues

    This needs to be indented to be aligned with the if. It is currently always executing because there is no break in the for loop

  5. 
      
mandeep
brennie
  1. 
      
  2. Show all issues

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

  3. djblets/util/templatetags/djblets_utils.py (Diff revisions 15 - 16)
     
     
     
    Show all issues

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

  4. Show all issues

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

  5. 
      
chipx86
  1. 
      
    1. By the way, this looks like a lot of comments, but most are just the same mistake or two made in multiple places. I wanted to point out the subtle ones so that you had a checklist and didn't have to go hunting for them yourself.

      Thanks for the contribution! Overall, this is a great addition, and is going to make things a lot easier in places!

  2. Show all issues

    This will actually be a django.template.RequestContext.

  3. djblets/util/templatetags/djblets_utils.py (Diff revision 16)
     
     
     
    Show all issues

    This should not be indented.

  4. djblets/util/templatetags/djblets_utils.py (Diff revision 16)
     
     
     
     
    Show all issues

    No blank line here.

  5. Show all issues

    The key=value should be in quotes, right?

    For "mode", is the equivalent always "update"? If so, we can say that explicitly. In fact, we can help further with examples:

    warnings.warn(
        '{% querystring_with "%(attr)s" "%(value)s" %} is deprecated ...'
        'Please use {% querystring "update" "%(attr)s=%(value)s" %} instead.'
        % {
            'key': attr,
            'value': value,
        },
        DeprecationWarning)
    
  6. Show all issues

    Too long for the line. Must fit in 79 characters.

    It's also missing an ending period.

  7. Show all issues

    See the type above.

  8. djblets/util/templatetags/djblets_utils.py (Diff revision 16)
     
     
     
    Show all issues

    This can be on the same line.

  9. djblets/util/templatetags/djblets_utils.py (Diff revision 16)
     
     
     
    Show all issues

    I think you can just do:

    query = context['request'].GET.copy()
    

    The copy will be mutable.

    1. If I do the above, the resulting query would be a dict and not be a QueryDict().

  10. djblets/util/templatetags/djblets_utils.py (Diff revision 16)
     
     
     
    Show all issues

    This is going to set the list of values for every value in the list of values. You should be able to remove the for loop here.

  11. djblets/util/templatetags/djblets_utils.py (Diff revision 16)
     
     
     
    Show all issues

    This doesn't seem right. We're overriding to_remove every time, meaning we're only ever getting the list for the last attribute. Was the rest of this meant to be within the for loop?

    This means to me that we're missing some very crucial unit tests somewhere.

  12. Show all issues

    This should raise a TemplateSyntaxError.

  13. Show all issues

    This is in the wrong import group.

  14. Show all issues

    The trailing period should remain. You only remove this for docstrings for unit tests themselves (as they're outputted to the terminal and shouldn't end in a period).

  15. djblets/util/tests/test_djblets_utils_tags.py (Diff revision 16)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  16. Show all issues

    Same here.

  17. Show all issues

    Needs a trailing period.

  18. Show all issues

    This should be removed.

  19. Show all issues

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

  20. Show all issues

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

  21. Show all issues

    Trailing comma.

  22. Show all issues

    """ on the next line.

    "overridden".

  23. Show all issues

    Trailing comma.

  24. djblets/util/tests/test_djblets_utils_tags.py (Diff revision 16)
     
     
     
     
     
     
    Show all issues

    Best to explicitly check the resulting value, rather than introducing another parsing/checking step. That just leads to problems, as more things can go wrong, and reviewers/future contributors have no idea what the result is even supposed to be.

    This applies to all tests.

  25. Show all issues

    "overridden"

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

  26. djblets/util/tests/test_djblets_utils_tags.py (Diff revision 16)
     
     
     
     
    Show all issues

    You can start the template on the first line, like:

    t = Template('{% load djblets_utils %}'
                 '{% querystring .... %}')
    

    Same for other tests.

  27. Show all issues

    Trailing comma.

  28. Show all issues

    Trailing comma.

  29. Show all issues

    Trailing comma.

  30. Show all issues

    Trailing comma.

  31. Show all issues

    """ on the next line.

    "overridden"

  32. Show all issues

    Trailing comma.

  33. Show all issues

    """ on the next line.

    No trailing period.

  34. Show all issues

    This should be removed.

  35. Show all issues

    "existing"

  36. Show all issues

    """ on the next line.

  37. Show all issues

    Trailing comma.

  38. Show all issues

    """ on the next line.

  39. Show all issues

    Trailing comma.

  40. Show all issues

    """ on the next line.

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

  41. Show all issues

    Trailing comma.

  42. Show all issues

    """ on the next line.

  43. Show all issues

    Trailing comma.

  44. Show all issues

    This should be removed.

  45. Show all issues

    """ on the next line.

  46. Show all issues

    Trailing comma.

  47. Show all issues

    "non-existing"

    """ on the next line.

  48. Show all issues

    Trailing comma.

  49. Show all issues

    """ on the next line.

  50. Show all issues

    Trailing comma.

  51. Show all issues

    """ on the next line.

  52. Show all issues

    Trailing comma.

  53. Show all issues

    """ on the next line.

  54. Show all issues

    Trailing comma.

  55. Show all issues

    """ on the next line.

  56. Show all issues

    Trailing comma.

  57. Show all issues

    """ on the next line.

  58. Show all issues

    Trailing comma.

  59. 
      
mandeep
brennie
  1. 
      
  2. djblets/util/templatetags/djblets_utils.py (Diff revision 17)
     
     
     
    Show all issues

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

  3. Show all issues

    Can you specify that this comes from this template tag, e.g.

    raise TemplateSyntaxError('Invalid mode for {%% querystring %%}: %s' % mode)
    
  4. Show all issues

    Single quotes here.

  5. 
      
mandeep
david
  1. Going to update this to use the new djblets.deprecation warnings and land it. Thanks!

  2. 
      
mandeep
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (e812f9d)