fix #181: allow FilterExpressions in bird tags

[benbacardi]

This is a draft PR for #181. It's a bit of a reshuffle for parameters, in particular the Value class, where raw is now expected to be a FilterExpression (from django.template.base).

This means we can now call components as such, and the Django filters are correctly parsed:

{% bird button badge_count=users|length %}
  Users
{% endbird %}

A summary of the changes:

• the "bits" from a bird tag (and a bird:prop tag) are parsed into key/value pairs split on =, where the value is a FilterExpression instance
• this FilterExpression instance is passed directly to the Value class when parsing the Params from the node
• when rendering, the FilterExpression is rendered with the context to provide the final value, and this greatly simplifies the Value.render method
• a couple of additional lines of logic is required to handle when a value is an attribute rather than a prop, so that things like disabled=False disappear from the attrs rather than appearing as disabled="False"—i.e. to mimic current behaviour.

There is one side effect that I'm not sure whether it's a problem or not. Currently, a prop passed with an in-template value of False ends up with a value of None inside the component, but with the above changes it will come through as it was originally with False.

My biggest bugbear with it is having to test the VariableDoesNotExist before calling the filter's resolve function, because the function itself has no way to stop it swallowing the error and returning the default empty string. I'd rather not do this, but it would mean no longer supporting unquoted raw strings. Personally I think that'd be an improvement, though…

I'm only considering the PR a draft because it clearly needs your input, @joshuadavidthomas — and I'm also not sure how best to rewrite all the test_params.py tests that don't expect FilterExpression instances all over the place! I've rejiggle a couple of the other tests (but locally the asset tests are also failing for seemingly unrelated reasons, so I'm not sure what will happen when they're run under CI).

All thoughts appreciated! I realise this isn't a "one-line change" and therefore a trivial PR to review/approve/reject.

[joshuadavidthomas]

Yeah, this is going to take me a minute to work through 😅

Not sure of when I'll be able to set aside some time to dive in, but I promise I'll get to it ASAP. Thanks for taking the time to dive in and contribute! 🎉💚

[joshuadavidthomas]

Promise I haven't forgotten about this. I've been down the Rust/LSP rabbit hole working on https://github.com/joshuadavidthomas/django-language-server with any spare time I have. Sorry for just letting this sit here! Thanks for taking the time to work on it (and for the nice write up in your blog post)!

[benbacardi]

No worries, I know what it's like! 🧡

[joshuadavidthomas]

Great work @benbacardi! Truly sorry for the enormous delay in putting together a proper review. I started it a few times, but couldn't push it over the edge to finish.

Left a few comments -- some nits, some questions, a couple small blockers -- nothing major here.

Before merge, we'll need some additional test cases with actual filters in the args for a bird tag as well as some documentation updates. I'm fine taking the docs on myself, though you're welcome a crack at it if you're up for it.

I realize given the time gulf between when you submitted this and my review now, you may have lost interest in finishing it which is totally understandable. I'll leave this open for you to finish if you would like, and if not I'll take it over in a week or two, make some gut decisions around my suggestions and merge it while crediting you.

Thanks again for the contribution and for your patience!

[joshuadavidthomas]

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]

[joshuadavidthomas]

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.

[joshuadavidthomas]

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

[joshuadavidthomas]

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.

[joshuadavidthomas]

Type hint needs adjustment slightly:

Suggested change

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

[joshuadavidthomas]

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.

[joshuadavidthomas]

I guess None would raise a VariableDoesNotExist exception, right?