-
Notifications
You must be signed in to change notification settings - Fork 18
Implement the 'first' filter #100
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
base: main
Are you sure you want to change the base?
Conversation
ca856bc
to
f98bb33
Compare
- Add FirstFilter struct and enum variant - Implement parsing logic for 'first' filter - Implement ResolveFilter trait with Django-compatible behavior - Returns first item of sequences (lists, tuples, strings) - Returns empty string for empty sequences - Raises TypeError for None and non-subscriptable types - Matches Django's exact error messages - Add comprehensive test suite porting all Django test cases - Test with lists, tuples, strings - Test empty sequences - Test autoescape on/off behavior - Test with mark_safe strings - Test error conditions (None, integers) - Add additional edge case tests refs LilyFirefly#1
f98bb33
to
284297f
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I have a few suggestions, but this is looking really good already.
- Added custom RenderError::NotSubscriptable for better error messages - Used inline isinstance check for PyIndexError - Implemented horizontal space reduction with early returns - Used str.split("") for efficient character iteration - Fixed all clippy warnings and formatting issues - Moved tests to tests/filters/test_first.py following project structure - All 18 tests pass with 100% Django compatibility The filter returns the first item of sequences (lists, tuples, strings) or empty string for empty sequences, matching Django's behavior exactly including error handling for non-subscriptable types. Co-Authored-By: Claude <[email protected]>
Following feedback on the first filter implementation, this commit adds proper source location tracking to the FirstFilter struct. The location is captured during parsing and used when reporting NotSubscriptable errors for non-sequence types (int and float). Changes: - Add `at: SourceSpan` field to FirstFilter struct - Add constructor method `FirstFilter::new(at: SourceSpan)` - Pass filter location from parser when creating FirstFilter instances - Use stored location instead of hardcoded (0, 0) in error reporting This provides more accurate error diagnostics by preserving the template position where the filter was applied, though the location is not yet displayed in the Python error messages. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Brings in CenterFilter and Bool content type support while maintaining FirstFilter location tracking
- Update FirstFilter to return Option<Content> to match new API - Add handling for Content::Bool variant in FirstFilter
The IntoOwnedContent trait is already imported at module scope, so the local import in FirstFilter::resolve is unnecessary.
- Inline s.as_raw() call to reduce unnecessary variable - More concise and readable code
a5acdf3
to
2cf99a1
Compare
- Implement WithSourceCode trait for PyTypeError to show miette error formatting - Update NotSubscriptable error handling to use the new method - Handle Python TypeError in FirstFilter to detect not subscriptable errors - Preserve custom TypeError messages while formatting standard ones - Now shows source code context with location indicators for type errors
Ensures complete coverage of all Content type branches
I think I've resolved all the open comments here 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is great progress. I've got a few more suggestions:
Err(e) if e.is_instance_of::<pyo3::exceptions::PyTypeError>(py) => { | ||
// Check if this is the standard "'type' object is not subscriptable" error | ||
let error_msg = e.to_string(); | ||
if error_msg.contains("is not subscriptable") { | ||
// Standard not subscriptable error - provide better formatting | ||
let type_name = obj | ||
.get_type() | ||
.name() | ||
.map(|n| n.to_string()) | ||
.unwrap_or_else(|_| "unknown".to_string()); | ||
Err(RenderError::NotSubscriptable { | ||
type_name, | ||
at: self.at, | ||
} | ||
.into()) | ||
} else { | ||
// Custom TypeError - preserve the original message | ||
Err(e.into()) | ||
} | ||
} | ||
Err(e) => Err(e.into()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since my last review, I've added a PyErr::annotate
method which I think we can use here:
Err(e) if e.is_instance_of::<pyo3::exceptions::PyTypeError>(py) => { | |
// Check if this is the standard "'type' object is not subscriptable" error | |
let error_msg = e.to_string(); | |
if error_msg.contains("is not subscriptable") { | |
// Standard not subscriptable error - provide better formatting | |
let type_name = obj | |
.get_type() | |
.name() | |
.map(|n| n.to_string()) | |
.unwrap_or_else(|_| "unknown".to_string()); | |
Err(RenderError::NotSubscriptable { | |
type_name, | |
at: self.at, | |
} | |
.into()) | |
} else { | |
// Custom TypeError - preserve the original message | |
Err(e.into()) | |
} | |
} | |
Err(e) => Err(e.into()), | |
Err(error) => { | |
let error = error.annotate(py, self.at, "here", template); | |
Err(error.into()) | |
} |
&self, | ||
variable: Option<Content<'t, 'py>>, | ||
py: Python<'py>, | ||
_template: TemplateString<'t>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_template: TemplateString<'t>, | |
template: TemplateString<'t>, |
|
||
#[derive(Clone, Debug, PartialEq)] | ||
pub struct FirstFilter { | ||
pub at: SourceSpan, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really matter, but I'd keep at
as a (usize, usize)
until using it in RenderError
:
pub at: SourceSpan, | |
pub at: (usize, usize), |
The reason is basically just to avoid using the third party type (miette::SourceSpan
) until it's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think this change will be necessary to use PyErr::annotate
.
}, | ||
#[error("'{type_name}' object is not subscriptable")] | ||
NotSubscriptable { | ||
type_name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can use &'static str
here to avoid an allocation:
type_name: String, | |
type_name: &'static str, |
Not a problem to leave it as is if this doesn't compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, and we can do this alongside the PyErr::annotate
change.
// Numbers are not sequences, should raise TypeError | ||
// Match Django's behavior exactly | ||
Err(RenderError::NotSubscriptable { | ||
type_name: "int".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If type_name
can be &'static str
, we can avoid the to_string()
call:
type_name: "int".to_string(), | |
type_name: "int", |
assert "'NoneType' object is not subscriptable" in str(django_exc.value) | ||
assert "'NoneType' object is not subscriptable" in str(rusty_exc.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to check the full error messages. Here's an example of what I mean:
Content::String(s) => { | ||
// For strings, get the first character | ||
// Skip the empty string always present at the start after splitting | ||
let mut chars = s.as_raw().split("").skip(1); | ||
match chars.next() { | ||
None => Ok(Some("".as_content())), | ||
Some(c) => Ok(Some(c.to_string().into_content())), | ||
} | ||
} | ||
Content::Int(_) => { | ||
// Numbers are not sequences, should raise TypeError | ||
// Match Django's behavior exactly | ||
Err(RenderError::NotSubscriptable { | ||
type_name: "int".to_string(), | ||
at: self.at, | ||
} | ||
.into()) | ||
} | ||
Content::Float(_) => { | ||
// Floats are not sequences, should raise TypeError | ||
// Match Django's behavior exactly | ||
Err(RenderError::NotSubscriptable { | ||
type_name: "float".to_string(), | ||
at: self.at, | ||
} | ||
.into()) | ||
} | ||
Content::Bool(_) => { | ||
// Booleans are not sequences, should raise TypeError | ||
// Match Django's behavior exactly | ||
Err(RenderError::NotSubscriptable { | ||
type_name: "bool".to_string(), | ||
at: self.at, | ||
} | ||
.into()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need tests for these.
Full disclosure upfront, this was generated with Claude Code. I'm excited about this project and some of this porting work felt like a good fit for an LLM. If this isn't welcome, feel free to close it. I'm happy to iterate on it if there is feedback.
Implementation Details
The filter returns the first item of a sequence (list, tuple, string) or empty string for empty sequences. It matches Django's behavior, including error handling.
Django References
Changes
FirstFilter
struct and enum variantResolveFilter
trait with Django-compatible behaviorTypeError
forNone
and non-subscriptable typesTesting
The implementation handles: