Skip to content

fix #181: allow FilterExpressions in bird tags #229

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix #181: allow FilterExpressions in bird tags #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/django_bird/_typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@

import sys

from django.template.base import FilterExpression

if sys.version_info >= (3, 12):
from typing import override as typing_override
else:
from typing_extensions import override as typing_override

override = typing_override

TagBits = list[str]
RawTagBits = list[str]
TagBits = dict[str, FilterExpression]
Comment on lines +14 to +15
Copy link
Owner

Choose a reason for hiding this comment

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

nit: I don't mind this, but I wonder if we could do a bit better to gain a bit more clarity about the relationship between these two types? E.g.:

Suggested change
RawTagBits = list[str]
TagBits = dict[str, FilterExpression]
RawTagBits = list[str]
ParsedTagBits = dict[str, FilterExpression]

5 changes: 2 additions & 3 deletions src/django_bird/components.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
from .conf import app_settings
from .params import Param
from .params import Params
from .params import Value
from .plugins import pm
from .staticfiles import Asset
from .staticfiles import AssetType
Expand Down Expand Up @@ -143,9 +142,9 @@ def render(self, context: Context):
data_attrs = [
Param(
f"data-bird-{self.component.data_attribute_name}",
Value(True),
True,
),
Param("data-bird-id", Value(f'"{self.component.id}-{self.id}"')),
Param("data-bird-id", f"{self.component.id}-{self.id}"),
]
self.params.attrs.extend(data_attrs)

Expand Down
66 changes: 22 additions & 44 deletions src/django_bird/params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
from typing import TYPE_CHECKING
from typing import Any

from django import template
from django.template.base import FilterExpression
from django.template.base import VariableDoesNotExist
from django.template.context import Context
from django.utils.safestring import SafeString
from django.utils.safestring import mark_safe
Expand Down Expand Up @@ -55,18 +56,21 @@ def render_attrs(self, context: Context) -> SafeString:
@classmethod
def from_node(cls, node: BirdNode) -> Params:
return cls(
attrs=[Param.from_bit(bit) for bit in node.attrs],
attrs=[Param(key, Value(value)) for key, value in node.attrs.items()],
props=[],
)


@dataclass
class Param:
name: str
value: Value
value: Value | str | bool
Copy link
Owner

Choose a reason for hiding this comment

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

HM! Feels like the abstraction is leaking a bit here.

I've become a recent convert to the thin dataclasses with more explicit names and a tagged union TypeAlias approach, I wonder if that could that could plug this leak up a bit.

@dataclass
class LiteralValue:
    raw: str

    def resolve(self, context: Context) -> str:
        return self.raw

@dataclass
class BooleanValue:
	value: bool

    def resolve(self, context: Context) -> bool | None:
        return self.value if self.value else None

@dataclass
class ExpressionValue:
	expression: FilterExpression


    def resolve(self, context: Context) -> Any:
        try:
            return self.expression.resolve(context)
        except VariableDoesNotExist:
            # Handle missing variables gracefully
            if self.expression.is_var and not self.expression.filters:
                return self.expression.token
            raise

Value = LiteralValue | BooleanValue | ExpressionValue

Looking through the rest of the code, I think this would be compatible with the rest of the calls, though that would obviously need to be tested.


def render_attr(self, context: Context) -> str:
value = self.value.resolve(context)
if isinstance(self.value, Value):
value = self.value.resolve(context, is_attr=True)
else:
value = self.value
Comment on lines +70 to +73
Copy link
Owner

Choose a reason for hiding this comment

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

If the tagged union approach is taken, this could just be reverted.

if value is None:
return ""
name = self.name.replace("_", "-")
Expand All @@ -75,47 +79,21 @@ def render_attr(self, context: Context) -> str:
return f'{name}="{value}"'

def render_prop(self, context: Context) -> str | bool | None:
return self.value.resolve(context)

@classmethod
def from_bit(cls, bit: str) -> Param:
if "=" in bit:
name, raw_value = bit.split("=", 1)
value = Value(raw_value.strip())
else:
name, value = bit, Value(True)
return cls(name, value)
return (
self.value.resolve(context) if isinstance(self.value, Value) else self.value
)


@dataclass
class Value:
raw: str | bool | None

def resolve(self, context: Context | dict[str, Any]) -> Any:
match (self.raw, self.is_quoted):
case (None, _):
return None

case (str(raw_str), False) if raw_str == "False":
return None
case (str(raw_str), False) if raw_str == "True":
return True

case (bool(b), _):
return b if b else None

case (str(raw_str), False):
try:
return template.Variable(raw_str).resolve(context)
except template.VariableDoesNotExist:
return raw_str

case (_, True):
return str(self.raw)[1:-1]

@property
def is_quoted(self) -> bool:
if self.raw is None or isinstance(self.raw, bool):
return False

return self.raw.startswith(("'", '"')) and self.raw.endswith(self.raw[0])
raw: FilterExpression

def resolve(self, context: Context | dict[str, Any], is_attr=False) -> Any:
if is_attr and self.raw.token == "False":
return None
if self.raw.is_var:
try:
self.raw.var.resolve(context)
except VariableDoesNotExist:
return self.raw.token
return self.raw.resolve(context)
14 changes: 10 additions & 4 deletions src/django_bird/templatetags/tags/bird.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from django.template.base import Token
from django.template.context import Context

from django_bird._typing import RawTagBits
from django_bird._typing import TagBits
from django_bird._typing import override

Expand All @@ -23,7 +24,7 @@ def do_bird(parser: Parser, token: Token) -> BirdNode:
raise template.TemplateSyntaxError(msg)

name = bits.pop(0)
attrs: TagBits = []
attrs: TagBits = {}
isolated_context = False

for bit in bits:
Expand All @@ -33,13 +34,18 @@ def do_bird(parser: Parser, token: Token) -> BirdNode:
case "/":
continue
case _:
attrs.append(bit)
if "=" in bit:
key, value = bit.split("=")
else:
key = bit
value = "True"
attrs[key] = parser.compile_filter(value)
Comment on lines +37 to +42
Copy link
Owner

@joshuadavidthomas joshuadavidthomas Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Need to make sure to only split on the first =, in case the bit has more than one e.g. {% bird button data-foo="bar=qux" %} -- a contrived example, but you get the idea.

Suggested change
if "=" in bit:
key, value = bit.split("=")
else:
key = bit
value = "True"
attrs[key] = parser.compile_filter(value)
if "=" in bit:
key, value = bit.split("=", 1)
else:
key = bit
value = "True"
attrs[key] = parser.compile_filter(value)

I think I'm guilty of doing the same thing elsewhere in the library, so I need to go through myself and make sure this edge case is handled correctly as well in any of those spots.


nodelist = parse_nodelist(bits, parser)
return BirdNode(name, attrs, nodelist, isolated_context)


def parse_nodelist(bits: TagBits, parser: Parser) -> NodeList | None:
def parse_nodelist(bits: RawTagBits, parser: Parser) -> NodeList | None:
# self-closing tag
# {% bird name / %}
if len(bits) > 0 and bits[-1] == "/":
Expand All @@ -55,7 +61,7 @@ class BirdNode(template.Node):
def __init__(
self,
name: str,
attrs: TagBits,
attrs: dict[str, str],
Copy link
Owner

Choose a reason for hiding this comment

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

Type hint needs adjustment slightly:

Suggested change
attrs: dict[str, str],
attrs: dict[str, FilterExpression],

nodelist: NodeList | None,
isolated_context: bool = False,
) -> None:
Expand Down
9 changes: 5 additions & 4 deletions src/django_bird/templatetags/tags/prop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from typing import final

from django import template
from django.template.base import FilterExpression
from django.template.base import Parser
from django.template.base import Token
from django.template.context import Context
Expand All @@ -14,7 +15,7 @@
TAG = "bird:prop"


def do_prop(_parser: Parser, token: Token) -> PropNode:
def do_prop(parser: Parser, token: Token) -> PropNode:
_tag, *bits = token.split_contents()
if not bits:
msg = f"{TAG} tag requires at least one argument"
Expand All @@ -26,14 +27,14 @@ def do_prop(_parser: Parser, token: Token) -> PropNode:
name, default = prop.split("=")
except ValueError:
name = prop
default = None
default = "None"
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder what the consequence of passing None vs "None" through as a FilterExpression? A quick glance at django.template.base.FilterExpression, I'm not totally sure. Need some tests to verify behavior is consistent and backwards compatible.

Copy link
Owner

Choose a reason for hiding this comment

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

I guess None would raise a VariableDoesNotExist exception, right?


return PropNode(name, default, bits)
return PropNode(name, parser.compile_filter(default), bits)


@final
class PropNode(template.Node):
def __init__(self, name: str, default: str | None, attrs: TagBits):
def __init__(self, name: str, default: FilterExpression, attrs: TagBits):
self.name = name
self.default = default
self.attrs = attrs
Expand Down
16 changes: 9 additions & 7 deletions tests/templatetags/test_bird.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ def test_missing_name_do_bird(self):
@pytest.mark.parametrize(
"params,expected_attrs",
[
('class="btn"', ['class="btn"']),
("class='btn'", ["class='btn'"]),
('class="btn" id="my-btn"', ['class="btn"', 'id="my-btn"']),
("disabled", ["disabled"]),
("class=dynamic", ["class=dynamic"]),
("class=item.name id=user.id", ["class=item.name", "id=user.id"]),
('class="btn"', {"class": '"btn"'}),
("class='btn'", {"class": "'btn'"}),
('class="btn" id="my-btn"', {"class": '"btn"', "id": '"my-btn"'}),
("disabled", {"disabled": "True"}),
("class=dynamic", {"class": "dynamic"}),
("class=item.name id=user.id", {"class": "item.name", "id": "user.id"}),
],
)
def test_attrs_do_bird(self, params, expected_attrs):
Expand All @@ -69,7 +69,9 @@ def test_attrs_do_bird(self, params, expected_attrs):

node = do_bird(parser, start_token)

assert node.attrs == expected_attrs
for key, value in node.attrs.items():
assert key in expected_attrs
assert expected_attrs[key] == value.token

@pytest.mark.parametrize(
"test_case",
Expand Down
5 changes: 3 additions & 2 deletions tests/templatetags/test_prop.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ def test_do_prop(contents, expected):
node = do_prop(Parser([]), start_token)

assert node.name == expected.name
assert node.default == expected.default
assert node.attrs == expected.attrs
assert (node.default.token if node.default else node.default) == expected.default
for node_attr, expected_attr in zip(node.attrs, expected.attrs, strict=False):
assert node_attr.token == expected_attr


def test_do_prop_no_args():
Expand Down
30 changes: 5 additions & 25 deletions tests/test_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,26 +121,6 @@ def test_render_attr(self, param, context, expected):
def test_render_prop(self, param, context, expected):
assert param.render_prop(context) == expected

@pytest.mark.parametrize(
"bit,expected",
[
("class='btn'", Param(name="class", value=Value("'btn'"))),
('class="btn"', Param(name="class", value=Value('"btn"'))),
("class=btn", Param(name="class", value=Value("btn"))),
("disabled", Param(name="disabled", value=Value(True))),
(
"class=item.name",
Param(name="class", value=Value("item.name")),
),
(
'class="item.name"',
Param(name="class", value=Value('"item.name"')),
),
],
)
def test_from_bit(self, bit, expected):
assert Param.from_bit(bit) == expected


class TestParams:
@pytest.mark.parametrize(
Expand Down Expand Up @@ -269,11 +249,11 @@ def test_render_attrs(self, params, context, expected):
"attrs,expected",
[
(
['class="btn"'],
{"class": '"btn"'},
Params(attrs=[Param(name="class", value=Value('"btn"'))]),
),
(
['class="btn"', 'id="my-btn"'],
{"class": '"btn"', "id": '"my-btn"'},
Params(
attrs=[
Param(name="class", value=Value('"btn"')),
Expand All @@ -282,15 +262,15 @@ def test_render_attrs(self, params, context, expected):
),
),
(
["disabled"],
{"disabled": True},
Params(attrs=[Param(name="disabled", value=Value(True))]),
),
(
["class=dynamic"],
{"class": "dynamic"},
Params(attrs=[Param(name="class", value=Value("dynamic"))]),
),
(
["class=item.name", "id=user.id"],
{"class": "item.name", "id": "user.id"},
Params(
attrs=[
Param(name="class", value=Value("item.name")),
Expand Down