Add support for Time.utc and Time.local #2410
Conversation
b-n
added
C-enhancement
These specs rely on Time.utc and Time.local. Previously, they were marked as NotImplemented, and thus the tests would continue to "pass" (it was more a false positive). inspect/to_s - Now requrie a Time.new method which isn't supported dst?/isdst - some issues to resolve getgm/getlocal - Requires parsing months as strings etc
| assert_eq!( | ||
| err.message().as_bstr(), | ||
| b"wrong number of arguments (given 0, expected 1..8)" | ||
| .as_slice() |
There was a problem hiding this comment.
bstr 1.2 has impls for [u8; N] so this as_slice isn't necessary anymore!
| } | ||
| Ok(result) | ||
| 10 => todo!(), | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
is rustc not smart enough to see that this match is exhaustive. 😢 if that's the case.
There was a problem hiding this comment.
Hah indeed. It's maybe just one level of abstraction too far for it to know that args.len() and the index on the iterator are the same thing I guess 🤷
There was a problem hiding this comment.
I have no concerns with the approach you've taken @b-n. It looks readable and well structured. The Args type keeps all the parsing local and easy to reason about, especially for these TODOs you've called out about supporting some of the polymorphic parameters.
Most of my comments are nits, not to do with the logic itself.
This makes progress towards #2223, however it does not yet close it. Breaking this work down to more atomic changes.
Notes:
"jan|feb|..."etcTryConvertMutsupporting two targets from&[Value]so the compiler now needs hintingTime.utc. This should be easily addable later, but happy to do it here too.Some design thoughts:
TryConvertMutblock, instead of in the accessor functions. I originally did it this way because I naively thought I couldto_intall the things, but apparently not (month param + sec taking fractional params)Time.newsupports azoneparameter from the looks of it)Value::valuein themruby.rsfile - since this feels more consistent with other implementations. That way, I can always say that trampoiline expects a RubyValue. However this limits the implementation a bit - e.g. right now we're doing some args length checking after coercion to RubyValues, where as I believe those checks should be done much earlier. Specificallly, I think it would be nicer if the new timeArgswas being generated directly frommruby.rsand being passed through. I'll comment in the code where I think this is applicable.Misc note: