From 5af8d6266600ceec4305121afe652a3da2d36f31 Mon Sep 17 00:00:00 2001 From: goldfish <37319612+ito-hiroki@users.noreply.github.com> Date: Sun, 7 Jul 2024 21:55:06 +0900 Subject: [PATCH] Fix `from toml` to handle toml datetime correctly (#13315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description fixed #12699 When bare dates or naive times are specified in toml files, `from toml` returns invalid dates or times. This PR fixes the problem to correctly handle toml datetime. The current version command returns the default datetime (`chrono::DateTime::default()`) if the datetime parse fails. However, I felt that this behavior was a bit unfriendly, so I changed it to return `Value::string`. # User-Facing Changes The command returns a date with default time and timezone if a bare date is specified. ``` ~/Development/nushell> "dob = 2023-05-27" | from toml ╭─────┬────────────╮ │ dob │ a year ago │ ╰─────┴────────────╯ ~/Development/nushell> "dob = 2023-05-27" | from toml | Sat, 27 May 2023 00:00:00 +0000 (a year ago) ~/Development/nushell> ``` If a bare time is given, a time string is returned. ``` ~/Development/nushell> "tm = 11:00:00" | from toml ╭────┬──────────╮ │ tm │ 11:00:00 │ ╰────┴──────────╯ ~/Development/nushell> "tm = 11:00:00" | from toml | get tm 11:00:00 ~/Development/nushell> ``` # Tests + Formatting When I ran tests, `commands::touch::change_file_mtime_to_reference` failed with the following error. The error also occurs in the master branch, so it's probably unrelated to these changes. (maybe a problem with my dev environment) ``` $ ~/Development/nushell> toolkit check pr ~~~~~~~~ test usage_start_uppercase ... ok test format_conversions::yaml::convert_dict_to_yaml_with_integer_floats_key ... ok test format_conversions::yaml::convert_dict_to_yaml_with_boolean_key ... ok test format_conversions::yaml::table_to_yaml_text_and_from_yaml_text_back_into_table ... ok test quickcheck_parse ... ok test format_conversions::yaml::convert_dict_to_yaml_with_integer_key ... ok failures: ---- commands::touch::change_file_mtime_to_reference stdout ---- === stderr thread 'commands::touch::change_file_mtime_to_reference' panicked at crates/nu-command/tests/commands/touch.rs:298:9: assertion `left == right` failed left: SystemTime { tv_sec: 1720344745, tv_nsec: 862392750 } right: SystemTime { tv_sec: 1720344745, tv_nsec: 887670417 } failures: commands::touch::change_file_mtime_to_reference test result: FAILED. 1542 passed; 1 failed; 32 ignored; 0 measured; 0 filtered out; finished in 12.04s error: test failed, to rerun pass `-p nu-command --test main` - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :red_circle: `toolkit test` - :black_circle: `toolkit test stdlib` ~/Development/nushell> toolkit test stdlib Compiling nu v0.95.1 (/Users/hiroki/Development/nushell) Compiling nu-cmd-lang v0.95.1 (/Users/hiroki/Development/nushell/crates/nu-cmd-lang) Finished dev [unoptimized + debuginfo] target(s) in 6.64s Running `target/debug/nu --no-config-file -c ' use crates/nu-std/testing.nu testing run-tests --path crates/nu-std '` 2024-07-07T19:00:20.423|INF|Running from_jsonl_invalid_object in module test_formats 2024-07-07T19:00:20.436|INF|Running env_log-prefix in module test_logger_env ~~~~~~~~~~~ 2024-07-07T19:00:22.196|INF|Running debug_short in module test_basic_commands ~/Development/nushell> ``` # After Submitting nothing --- crates/nu-command/src/formats/from/toml.rs | 193 +++++++++++++++++---- 1 file changed, 159 insertions(+), 34 deletions(-) diff --git a/crates/nu-command/src/formats/from/toml.rs b/crates/nu-command/src/formats/from/toml.rs index e1ddca3164..266911f50a 100644 --- a/crates/nu-command/src/formats/from/toml.rs +++ b/crates/nu-command/src/formats/from/toml.rs @@ -1,5 +1,5 @@ use nu_engine::command_prelude::*; -use std::str::FromStr; +use toml::value::{Datetime, Offset}; #[derive(Clone)] pub struct FromToml; @@ -56,6 +56,54 @@ b = [1, 2]' | from toml", } } +fn convert_toml_datetime_to_value(dt: &Datetime, span: Span) -> Value { + match &dt.clone() { + toml::value::Datetime { + date: Some(_), + time: _, + offset: _, + } => (), + _ => return Value::string(dt.to_string(), span), + } + + let date = match dt.date { + Some(date) => { + chrono::NaiveDate::from_ymd_opt(date.year.into(), date.month.into(), date.day.into()) + } + None => Some(chrono::NaiveDate::default()), + }; + + let time = match dt.time { + Some(time) => chrono::NaiveTime::from_hms_nano_opt( + time.hour.into(), + time.minute.into(), + time.second.into(), + time.nanosecond, + ), + None => Some(chrono::NaiveTime::default()), + }; + + let tz = match dt.offset { + Some(offset) => match offset { + Offset::Z => chrono::FixedOffset::east_opt(0), + Offset::Custom { minutes: min } => chrono::FixedOffset::east_opt(min as i32 * 60), + }, + None => chrono::FixedOffset::east_opt(0), + }; + + let datetime = match (date, time, tz) { + (Some(date), Some(time), Some(tz)) => chrono::NaiveDateTime::new(date, time) + .and_local_timezone(tz) + .earliest(), + _ => None, + }; + + match datetime { + Some(datetime) => Value::date(datetime, span), + None => Value::string(dt.to_string(), span), + } +} + fn convert_toml_to_value(value: &toml::Value, span: Span) -> Value { match value { toml::Value::Array(array) => { @@ -76,13 +124,7 @@ fn convert_toml_to_value(value: &toml::Value, span: Span) -> Value { span, ), toml::Value::String(s) => Value::string(s.clone(), span), - toml::Value::Datetime(d) => match chrono::DateTime::from_str(&d.to_string()) { - Ok(nushell_date) => Value::date(nushell_date, span), - // in the unlikely event that parsing goes wrong, this function still returns a valid - // nushell date (however the default one). This decision was made to make the output of - // this function uniform amongst all eventualities - Err(_) => Value::date(chrono::DateTime::default(), span), - }, + toml::Value::Datetime(dt) => convert_toml_datetime_to_value(dt, span), } } @@ -113,32 +155,6 @@ mod tests { test_examples(FromToml {}) } - #[test] - fn from_toml_creates_nushell_date() { - let toml_date = toml::Value::Datetime(Datetime { - date: Option::from(toml::value::Date { - year: 1980, - month: 10, - day: 12, - }), - time: Option::from(toml::value::Time { - hour: 10, - minute: 12, - second: 44, - nanosecond: 0, - }), - offset: Option::from(toml::value::Offset::Custom { minutes: 120 }), - }); - - let span = Span::test_data(); - let reference_date = Value::date(Default::default(), Span::test_data()); - - let result = convert_toml_to_value(&toml_date, span); - - //positive test (from toml returns a nushell date) - assert_eq!(result.get_type(), reference_date.get_type()); - } - #[test] fn from_toml_creates_correct_date() { let toml_date = toml::Value::Datetime(Datetime { @@ -206,4 +222,113 @@ mod tests { assert!(result.is_err()); } + + #[test] + fn convert_toml_datetime_to_value_date_time_offset() { + let toml_date = Datetime { + date: Option::from(toml::value::Date { + year: 2000, + month: 1, + day: 1, + }), + time: Option::from(toml::value::Time { + hour: 12, + minute: 12, + second: 12, + nanosecond: 0, + }), + offset: Option::from(toml::value::Offset::Custom { minutes: 120 }), + }; + + let span = Span::test_data(); + let reference_date = Value::date( + chrono::FixedOffset::east_opt(60 * 120) + .unwrap() + .with_ymd_and_hms(2000, 1, 1, 12, 12, 12) + .unwrap(), + span, + ); + + let result = convert_toml_datetime_to_value(&toml_date, span); + + assert_eq!(result, reference_date); + } + + #[test] + fn convert_toml_datetime_to_value_date_time() { + let toml_date = Datetime { + date: Option::from(toml::value::Date { + year: 2000, + month: 1, + day: 1, + }), + time: Option::from(toml::value::Time { + hour: 12, + minute: 12, + second: 12, + nanosecond: 0, + }), + offset: None, + }; + + let span = Span::test_data(); + let reference_date = Value::date( + chrono::FixedOffset::east_opt(0) + .unwrap() + .with_ymd_and_hms(2000, 1, 1, 12, 12, 12) + .unwrap(), + span, + ); + + let result = convert_toml_datetime_to_value(&toml_date, span); + + assert_eq!(result, reference_date); + } + + #[test] + fn convert_toml_datetime_to_value_date() { + let toml_date = Datetime { + date: Option::from(toml::value::Date { + year: 2000, + month: 1, + day: 1, + }), + time: None, + offset: None, + }; + + let span = Span::test_data(); + let reference_date = Value::date( + chrono::FixedOffset::east_opt(0) + .unwrap() + .with_ymd_and_hms(2000, 1, 1, 0, 0, 0) + .unwrap(), + span, + ); + + let result = convert_toml_datetime_to_value(&toml_date, span); + + assert_eq!(result, reference_date); + } + + #[test] + fn convert_toml_datetime_to_value_only_time() { + let toml_date = Datetime { + date: None, + time: Option::from(toml::value::Time { + hour: 12, + minute: 12, + second: 12, + nanosecond: 0, + }), + offset: None, + }; + + let span = Span::test_data(); + let reference_date = Value::string(toml_date.to_string(), span); + + let result = convert_toml_datetime_to_value(&toml_date, span); + + assert_eq!(result, reference_date); + } }