Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #34578 -- Made "join" template filter respect autoescape for joiner. #16873

Merged
merged 1 commit into from
May 19, 2023

Conversation

rajeeshrp
Copy link

  • If autoescape is off, join filter will not escape the joining string now.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@rajeeshrp Thanks 👍 I left comments.

tests/template_tests/filter_tests/test_join.py Outdated Show resolved Hide resolved
tests/template_tests/filter_tests/test_join.py Outdated Show resolved Hide resolved
tests/template_tests/filter_tests/test_join.py Outdated Show resolved Hide resolved
tests/template_tests/filter_tests/test_join.py Outdated Show resolved Hide resolved
@felixxm felixxm changed the title Fixed #34578 Fixed #34578 -- Made join template filter respect autoescape for joiner. May 19, 2023
@felixxm felixxm changed the title Fixed #34578 -- Made join template filter respect autoescape for joiner. Fixed #34578 -- Made "join" template filter respect autoescape for joiner. May 19, 2023
@felixxm
Copy link
Member

felixxm commented May 19, 2023

@rajeeshrp Thanks 👍 Welcome aboard ⛵

I pushed small edits and squashed commits.

@shangxiao
Copy link
Contributor

I was going to make a small suggestion unrelated to the ticket in the function but I'll wait until you've merged it and submit a PR separately 🤔

@felixxm
Copy link
Member

felixxm commented May 19, 2023

I was going to make a small suggestion unrelated to the ticket in the function but I'll wait until you've merged it and submit a PR separately thinking

Please share 👀

@shangxiao
Copy link
Contributor

shangxiao commented May 19, 2023

Oh hm after your edit it doesn't really refactor that well but going on the previous version (and this was a thing in the original code so no criticism of rajeeshp here):

@register.filter(is_safe=True, needs_autoescape=True)
def join(value, arg, autoescape=True):
    """Join a list with a string, like Python's ``str.join(list)``."""
    try:
        if autoescape:
            data = conditional_escape(arg).join(conditional_escape(v) for v in value)
        else:
            data = arg.join(value)
    except TypeError:  # Fail silently if arg isn't iterable.
        return value
    return mark_safe(data)

The point is to use a generator expression instead of a material list:

  • slight optimisation for what that's worth
  • I strongly discourage the reassignment of variables (especially when there's a type change) as it can lead to brittle code

You could also "improve" if further with the data = <expr1> if <condition> else <expr2> expression 🤷‍♂️ ("improve" in this case is entirely subjective)

@felixxm
Copy link
Member

felixxm commented May 19, 2023

The point is to use a generator expression instead of a material list:

A list comprehension is preferable here as str.join() converts to list internally anyway. It is better performance to provide a list up front.

@felixxm
Copy link
Member

felixxm commented May 19, 2023

I strongly discourage the reassignment of variables (especially when there's a type change) as it can lead to brittle code

Agreed 👍

@shangxiao
Copy link
Contributor

Well TIL! ❤️

@felixxm felixxm merged commit a2da81f into django:main May 19, 2023
24 checks passed
@rajeeshrp rajeeshrp deleted the ticket_34578 branch May 19, 2023 13:41
@felixxm felixxm temporarily deployed to schedules May 20, 2023 02:44 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants