Skip to content

Conversation

chinmoy12c
Copy link
Contributor

@chinmoy12c chinmoy12c force-pushed the ticket_6135 branch 7 times, most recently from dc54b67 to c8f8cf5 Compare August 21, 2021 11:43
@chinmoy12c
Copy link
Contributor Author

@felixxm can you take a look at this. Thanks :).

@chinmoy12c chinmoy12c force-pushed the ticket_6135 branch 4 times, most recently from d6fe30c to 6f527d6 Compare October 5, 2021 15:03
@chinmoy12c chinmoy12c force-pushed the ticket_6135 branch 6 times, most recently from cc07cd7 to 5691a43 Compare January 15, 2022 09:37
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up @chinmoy12c.

I have to say that after 14 years I think we should probably close this as wontfix. 😬

I think the new decorator is likely to cause more confusion that it'll solve.

Specifically, it's only going to be appropriate for filters already marked @stringfilter — that's fine, in theory, but that's not documented at all — and it's going to be anything but clear to document — and we're going to see a whole load of reports saying @autoescape_aware doesn't work with my filter.

There's already a way to do this, and the proposed decorator isn't sufficiently robust to cover all cases.

Also, as evidenced by those 14 years, I'm not convinced the saving is all that great:

    if autoescape: 
        esc = conditional_escape 
    else: 
        esc = lambda x: x 

… OK, yes. It's slightly repetitive. If it really bothered me I could write a make_escape() factory function to remove the duplication. But, on the other hand, it's clear: there's no question of what the behaviour is, and I don't have to go looking into the source to see how the parameters get applied.

Summary, I'm not at all convinced the proposed decorator pays its way.
🤔

@carltongibson
Copy link
Member

Closing as per ticket. Thanks @chinmoy12c. Sorry. 🎁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants