Feat/yesno filter #78
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
==========================================
- Coverage 98.56% 98.37% -0.20%
==========================================
Files 43 44 +1
Lines 5853 6016 +163
Branches 1913 1965 +52
==========================================
+ Hits 5769 5918 +149
- Misses 63 77 +14
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi!
Awesome!
I'm sure that won't be a problem.
I'm happy to try and answer any questions you have. 😄
For now, I don't really mind whether the tests are written in Rust or in Python, so long as we have (close to) 100% coverage in CI. |
| None => return Err(PyRenderError::RenderError(RenderError::VariableDoesNotExist { | ||
| key: "yesno".to_string(), | ||
| object: "filter".to_string(), | ||
| key_at: (0, 0).into(), | ||
| object_at: None, | ||
| })), |
There was a problem hiding this comment.
Can we test this code path?
You can find an example of the sort of way I'd test errors here:
https://github.com/LilyFoote/django-rusty-templates/blob/a7e3e9fc524ea65a5fc9c91eeb52ce7f6c815f07/tests/filters/test_add.py#L25-L45
| let mapping = match arg_content.to_py(py)?.extract::<String>() { | ||
| Ok(s) => s, | ||
| Err(_) => { | ||
| return Err(PyRenderError::RenderError(RenderError::VariableDoesNotExist { | ||
| key: "yesno argument".to_string(), | ||
| object: "string".to_string(), | ||
| key_at: (0, 0).into(), | ||
| object_at: None, | ||
| })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I think we shouldn't need to call to_py here. Can we instead use resolve_string?
Something like:
| let mapping = match arg_content.to_py(py)?.extract::<String>() { | |
| Ok(s) => s, | |
| Err(_) => { | |
| return Err(PyRenderError::RenderError(RenderError::VariableDoesNotExist { | |
| key: "yesno argument".to_string(), | |
| object: "string".to_string(), | |
| key_at: (0, 0).into(), | |
| object_at: None, | |
| })); | |
| } | |
| }; | |
| let mapping = arg_content.resolve_string(context)?.content(); |
Otherwise, we should probably match arg_content and handle each case directly.
|
|
||
| def test_yesno_two_options(self): | ||
| # Test with only two options - None uses the second option | ||
| template = Template("{{ var|yesno:'yep,nah' }}") |
There was a problem hiding this comment.
We have tests for three and for two mappings. We should also test that we match Django's behaviour for zero, one or more than three mappings.
| let result = if parts.len() >= 3 { | ||
| parts[2].to_string() | ||
| } else { | ||
| parts.get(1).unwrap_or(&"no").to_string() | ||
| }; | ||
| return Ok(Some(Content::String(Cow::Owned(result)))); |
There was a problem hiding this comment.
I think it would be cleaner to match on the length of parts and handle each case explicitly:
| let result = if parts.len() >= 3 { | |
| parts[2].to_string() | |
| } else { | |
| parts.get(1).unwrap_or(&"no").to_string() | |
| }; | |
| return Ok(Some(Content::String(Cow::Owned(result)))); | |
| let result = match parts.len() { | |
| 3 => Cow::Owned(parts[2].to_string()), | |
| 2 => Cow::Owned(parts[1].to_string()), | |
| 1 => Cow::Borrowed("no"), | |
| 0 => todo!("Match Django's behaviour"), | |
| _ => todo!("Match Django's behaviour"), | |
| }; | |
| return Ok(Some(Content::String(result))); |
| let parts: Vec<&str> = mapping.split(',').collect(); | ||
|
|
||
| // Handle None values | ||
| if left.to_py(py)?.is_none() { |
There was a problem hiding this comment.
I wonder if we should add a new method to Content returning an enum with three variants: None, Truthy and Falsey, which we can then match on here.
Co-authored-by: Lily Foote <[email protected]>
Co-authored-by: Lily Foote <[email protected]>
Co-authored-by: Lily Foote <[email protected]>
Co-authored-by: Lily Foote <[email protected]>
Co-authored-by: Lily Foote <[email protected]>
Hello!
I got really interested in the project a few weeks back, and I thought I just have a go at contributing.
I honestly have no experience in rust, so if it gets to a point where my contributions are more a net negative, just let me know.
I don't really understand most parts of the project, and I even have some basic questions. Maybe if I pick one for now: If I'm going to implement several of the filters, do we really need the rust tests in addition to the python tests? The filter usually don't have any fancy logic, and then I personally would be fine with just testing the "blackbox" behavior with the python tests.