Code

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#3838 closed (wontfix)

stringformat filter allows trailing non-format characters but not leading characters

Reported by: msimoens@… Owned by: nobody
Component: Template system Version: master
Severity: Keywords: stringformat
Cc: msimoens@…, lakin.wecker@…, jm.bugtracking@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This filter seems to provide an, intentionally simplified, implementation of python's format strings. I'm not sure what the intent was with this simplification, however it is inconsistent. Due to the requirement that the preceding '%' be left out, one cannot put leading constant characters into the format string (which may be intended). However, one _can_ put trailing constant characters into the format string and it will still work. For instance:

{# fails #}
{{ generic_key_type|stringformat:"course/%s.html" }} 

{# works #}
{{ generic_key_type|stringformat:"s/course/index.html" }} 


The first way will not work, however the second way will work, which is inconsistent. Either the filter should ensure that no trailing constant characters are used or it should allow both ways. If the intent is to simplify it in some way, then it should consistently not allow either trailing or leading characters. In this case, the name is very misleading as the filter is not equivalent to python's format strings, and I would argue that it should be renamed after the subset of functionality that it is intended to provide.

With its current name, I would argue that it should allow both trailing and leading characters, and the following is an implementation that allows this, and attempts to remain backwards compatible.

def stringformat(value, arg):
    """
    Formats the variable according to the argument, a string formatting specifier.
    This specifier uses Python string formating syntax with the exception that
    the leading "%" may be dropped.

    See http://docs.python.org/lib/typesseq-strings.html for documentation
    of Python string formatting
    """
    
    try:
        return ("%" + str(arg)) % value
    except (ValueError, TypeError):
        try: 
            return str(arg) % value
        except (ValueError, TypeError):
            return ""

Attachments (0)

Change History (7)

comment:1 Changed 7 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

This filter is really more for alternate types of formatting (eg |stringfilter:".2f") than string concatenation.

Your alternate method seems a bit too magic, and still isn't going to work with cases like |stringformat:"settings/%s.html".

comment:2 Changed 7 years ago by lakin.wecker@…

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I'm re-opening this report, as the resolution is not appropriate given the answer that was posted. I respectfully disagree with this being a "wontfix". If this filter

... is really more for alternate types of formatting (eg |stringfilter:".2f") than string concatenation.

Then there is a bug ... Because it does allow for string concatenation. Hence there is a bug to be fixed. Mark it as low priority if you'd like, but it's still a bug. More importantly, I agree with the original poster that if this is the intent, then the filter is mis-named and mis-documented. It needs attention.

The proposed method is no more "magic" than the original implementation, and it _will_ handle the case that you brought up:

|stringformat:"settings/%s.html"

comment:3 Changed 7 years ago by SmileyChris

Ok, fair enough - it does handle the case I brought up because that raises a TypeError.

I'll defer the decision to a core developer (and thanks to lakin's correction of my interpretation, my opinion changes is +0)

comment:4 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

comment:5 Changed 7 years ago by anonymous

  • Cc jm.bugtracking@… added

comment:6 Changed 7 years ago by ubernostrum

  • Resolution set to wontfix
  • Status changed from reopened to closed

Going back to wontfix for the reasons Chris gave originally; the fact that you can, in some situations, accidentally use something in a way that's undocumented and unsupported does not mean we should be fixing bugs for that use.

comment:7 Changed 7 years ago by jm.bugtracking@…

but you should fix it so that it can't be used in ways that are undocumented and unsupported. I still find the naming ambiguous and frankly think that a filter should does what it name says, not what the documentation says that it currently does, especially if both have a big overlap as it is now.

So... if stringformat can't be fixed so that it allows all possible uses (and adding documentation for those, as proposed here), then it probably should be changed in a way that raises an appropriate exception with a meaningful error message, or not?

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.