alamb commented on code in PR #4581:
URL: https://github.com/apache/arrow-rs/pull/4581#discussion_r1278543245
##########
arrow-cast/src/display.rs:
##########
@@ -866,6 +951,15 @@ pub fn lexical_to_string<N: lexical_core::ToLexical>(n: N)
-> String {
mod tests {
use super::*;
+ const TEST_CONST_OPTIONS: FormatOptions<'static> = FormatOptions::new()
Review Comment:
I think a comment explaining the rationale of the test (so it isn't
inadvertently remove) would be helpful
```suggestion
// Test to verify options can be constant. See
// https://github.com/apache/arrow-rs/issues/4580
const TEST_CONST_OPTIONS: FormatOptions<'static> = FormatOptions::new()
```
##########
arrow-cast/src/display.rs:
##########
@@ -68,64 +76,79 @@ impl<'a> Default for FormatOptions<'a> {
timestamp_format: None,
timestamp_tz_format: None,
time_format: None,
+ iso8601_duration_format: true,
}
}
-}
-impl<'a> FormatOptions<'a> {
/// If set to `true` any formatting errors will be written to the output
/// instead of being converted into a [`std::fmt::Error`]
- pub fn with_display_error(mut self, safe: bool) -> Self {
+ pub const fn with_display_error(mut self, safe: bool) -> Self {
self.safe = safe;
self
}
/// Overrides the string used to represent a null
///
/// Defaults to `""`
- pub fn with_null(self, null: &'a str) -> Self {
+ pub const fn with_null(self, null: &'a str) -> Self {
Self { null, ..self }
}
/// Overrides the format used for [`DataType::Date32`] columns
- pub fn with_date_format(self, date_format: Option<&'a str>) -> Self {
+ pub const fn with_date_format(self, date_format: Option<&'a str>) -> Self {
Self {
date_format,
..self
}
}
/// Overrides the format used for [`DataType::Date64`] columns
- pub fn with_datetime_format(self, datetime_format: Option<&'a str>) ->
Self {
+ pub const fn with_datetime_format(self, datetime_format: Option<&'a str>)
-> Self {
Self {
datetime_format,
..self
}
}
/// Overrides the format used for [`DataType::Timestamp`] columns without
a timezone
- pub fn with_timestamp_format(self, timestamp_format: Option<&'a str>) ->
Self {
+ pub const fn with_timestamp_format(self, timestamp_format: Option<&'a
str>) -> Self {
Self {
timestamp_format,
..self
}
}
/// Overrides the format used for [`DataType::Timestamp`] columns with a
timezone
- pub fn with_timestamp_tz_format(self, timestamp_tz_format: Option<&'a
str>) -> Self {
+ pub const fn with_timestamp_tz_format(
+ self,
+ timestamp_tz_format: Option<&'a str>,
+ ) -> Self {
Self {
timestamp_tz_format,
..self
}
}
/// Overrides the format used for [`DataType::Time32`] and
[`DataType::Time64`] columns
- pub fn with_time_format(self, time_format: Option<&'a str>) -> Self {
+ pub const fn with_time_format(self, time_format: Option<&'a str>) -> Self {
Self {
time_format,
..self
}
}
+
+ /// If true formats durations as ISO8601, otherwise prints a human
readable format
+ ///
+ /// * ISO 8601 - "P198DT72932.972880S"
+ /// * Human Readable - "198 days 16 hours 34 mins 15.407810000 secs"
+ ///
+ /// Defaults to true
+ pub const fn with_iso8601_duration_format(self, iso8601: bool) -> Self {
Review Comment:
Rather than a `bool` here, an enum that would allow more easily adding
different formats in the future without changing the API.
Perhaps something like
```rust
enum DurationFormat {
/// Human Readable - "198 days 16 hours 34 mins 15.407810000 secs"
Human,
/// ISO 8601 - "P198DT72932.972880S"
ISO8601,
}
...
pub const with_duration_format(mut self, format: DurationFormat) {
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
