feat: Raise informative error instead of panicking when passing invalid directives to to_string for Date dtype
#17670
Conversation
MarcoGorelli
changed the title
to_string for Date dtypeto_string for Date dtype
github-actions
bot
added
enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17670 +/- ##
=======================================
Coverage 80.49% 80.49%
=======================================
Files 1503 1503
Lines 197054 197049 -5
Branches 2805 2805
=======================================
- Hits 158615 158613 -2
+ Misses 37918 37915 -3
Partials 521 521 ☔ View full report in Codecov by Sentry. |
MarcoGorelli
requested review from
ritchie46,
stinodego,
c-peters,
alexander-beedie,
reswqa and
orlp
as code owners
MarcoGorelli
changed the title
to_string for Date dtypeto_string for Date dtype
MarcoGorelli
changed the title
to_string for Date dtypeto_string for Date dtype
…ng unsupported directives to `.dt.to_string`
| @@ -115,6 +115,55 @@ where | |||
|
|
|||
| ChunkedArray::try_from_chunk_iter(self.name(), iter) | |||
| } | |||
|
|
|||
| pub fn apply_to_buffer_generic<'a, F>(&'a self, mut f: F) -> StringChunked | |||
There was a problem hiding this comment.
Except, it shouldn't be called this, as it is not generic. It returns a StringChunked/
Maybe apply_to_string_amortized?
There was a problem hiding this comment.
true, I was just thinking that, respect to apply_to_buffer, the input can be any PolarsDataType
I think apply_to_string_amortized makes it sounds like the input is a String?
EDIT I guess apply_to_struct also follows this naming pattern...OK, updating 👍
There was a problem hiding this comment.
Would apply_into_string be an OK name? apply_to_string to me just reads like the string is the input rather than the output
There was a problem hiding this comment.
Sure, but change the apply_to_struct and other as well then.
| .downcast_iter() | ||
| .map(|arr| { | ||
| let mut mutarr = MutablePlString::with_capacity(arr.len()); | ||
| arr.iter().try_for_each(|opt| { |
There was a problem hiding this comment.
Can we make this just a loop? It's a bit less cluttered with the fallible return types.
closes #16271
I've added
apply_to_buffer_genericandtry_apply_to_buffer_generictoo as they'd be very useful to plugin authors (expecting them to manually re-implement this is probably unrealistic, and from tests I've done, allocating on each iteration slows things down by 3-4x)