fix #181: allow FilterExpressions in bird tags#229
fix #181: allow FilterExpressions in bird tags#229joshuadavidthomas merged 3 commits intojoshuadavidthomas:mainfrom
Conversation
|
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! 🎉💚 |
|
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)! |
|
No worries, I know what it's like! 🧡 |
There was a problem hiding this comment.
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!
| self, | ||
| name: str, | ||
| attrs: TagBits, | ||
| attrs: dict[str, str], |
There was a problem hiding this comment.
Type hint needs adjustment slightly:
| attrs: dict[str, str], | |
| attrs: dict[str, FilterExpression], |
|
@benbacardi thank you again for the original implementation here — and I’m sorry I let this PR sit for so long. I’ve pushed the remaining updates to get this over the line:
I kept scope focused on landing this safely, and left broader refactor ideas for follow-up work. Thanks again for the contribution — this feature exists because of your work. |
5fe2fc3 to
81a7b93
Compare
6304d6b to
2493b64
Compare
Two recent PRs introduced uncovered lines that pushed coverage from ~98.5% to 97.68%: - `ab7f667` (fix 3.14) added a Windows-only fallback in templates.py that can't be reached on Linux CI — mark with `pragma: no cover` - `82c433be` (#229 FilterExpressions) added defensive code paths in params.py — add tests for the catch-all TypeError case and the `is_attr=True` with `False` value branch https://claude.ai/code/session_01Rfs15Qp5BaeXiK8TLExfe7
* Fix code coverage dropping below 98% threshold Two recent PRs introduced uncovered lines that pushed coverage from ~98.5% to 97.68%: - `ab7f667` (fix 3.14) added a Windows-only fallback in templates.py that can't be reached on Linux CI — mark with `pragma: no cover` - `82c433be` (#229 FilterExpressions) added defensive code paths in params.py — add tests for the catch-all TypeError case and the `is_attr=True` with `False` value branch https://claude.ai/code/session_01Rfs15Qp5BaeXiK8TLExfe7 * Raise coverage threshold to 100% with tests for all uncovered lines Adds tests covering 15 previously uncovered lines across 5 files: - apps.py: pre_ready/ready plugin hook execution - components.py: fallback name when template is outside component dirs - manifest.py: ValueError in normalize_path when path is prefix of BASE_DIR - params.py: render_props early return when component.nodelist is None - staticfiles.py: AssetTypes.reset, Asset.render/absolute_path, get_component_assets filtering, BirdAssetStorage.url(None), BirdAssetFinder legacy all param, BirdAssetFinder.list error handling https://claude.ai/code/session_01Rfs15Qp5BaeXiK8TLExfe7 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This is a draft PR for #181. It's a bit of a reshuffle for parameters, in particular the
Valueclass, whererawis now expected to be aFilterExpression(fromdjango.template.base).This means we can now call components as such, and the Django filters are correctly parsed:
A summary of the changes:
birdtag (anda bird:proptag) are parsed into key/value pairs split on=, where the value is aFilterExpressioninstanceFilterExpressioninstance is passed directly to theValueclass when parsing theParamsfrom the nodeFilterExpressionis rendered with the context to provide the final value, and this greatly simplifies theValue.rendermethoddisabled=Falsedisappear from theattrsrather than appearing asdisabled="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
Falseends up with a value ofNoneinside the component, but with the above changes it will come through as it was originally withFalse.My biggest bugbear with it is having to test the
VariableDoesNotExistbefore 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.pytests that don't expectFilterExpressioninstances 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.