Implement the datetime plan with fixed offsets to start#171
Implement the datetime plan with fixed offsets to start#171mwildehahn wants to merge 40 commits intopydantic:mainfrom
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
crates/monty-python/src/convert.rs
Outdated
| Ok(MontyObject::String(string.extract()?)) | ||
| } else if let Ok(bytes) = obj.cast::<PyBytes>() { | ||
| Ok(MontyObject::Bytes(bytes.extract()?)) | ||
| } else if obj.is_instance(get_datetime_datetime(obj.py())?)? { |
There was a problem hiding this comment.
the cpython c-api and pyo3 have proper support python datetime objects, e.g. PyDatetime, you should cast to those here.
You might want to look at the code in pydantic for how we do this there.
| # def __new__(cls: type[Self], ...) -> Self: ... | ||
| # In other cases, use `typing_extensions.Self`. | ||
| Self = TypeVar('Self') | ||
| Self = TypeVar('Self') # noqa: Y001 |
There was a problem hiding this comment.
please undo all these changes in crates/monty-typeshed/vendor/typeshed/stdlib/
There was a problem hiding this comment.
@samuelcolvin to get tests passing I had to regen typeshed. LMK if there is a better way to do this or if you still want me to remove these.
crates/monty/src/bytecode/vm/call.rs
Outdated
| match (t, method_id) { | ||
| (Type::Dict, m) if m == StaticStrings::Fromkeys => return dict_fromkeys(args, heap, interns), | ||
| (Type::Bytes, m) if m == StaticStrings::Fromhex => return bytes_fromhex(args, heap, interns), | ||
| (Type::Dict, m) if m == StaticStrings::Fromkeys => { |
There was a problem hiding this comment.
we should probably move these methods into dict.rs etc. rather than have the logic here.
| /// Returns datetime awareness (`true` for aware, `false` for naive) for datetime values. | ||
| /// | ||
| /// Returns `None` when the value is not a datetime reference. | ||
| fn datetime_awareness(value: &Value, heap: &crate::heap::Heap<impl ResourceTracker>) -> Option<bool> { |
There was a problem hiding this comment.
I think this method is duplicated.
crates/monty/src/run.rs
Outdated
| // Pre-process OS callback results before restoring the VM to avoid borrowing | ||
| // conflicts with the VM's mutable borrow of `self.heap`. | ||
| let resume_action = match ext_result { | ||
| ExternalResult::Return(obj) => { | ||
| if let Some(transform) = self.pending_os_transform { | ||
| match transform_os_return_value(obj, transform, &mut self.heap) { | ||
| Ok(value) => ResumeAction::ReturnValue(value), | ||
| Err(msg) => ResumeAction::Error(RunError::from(MontyException::runtime_error(format!( | ||
| "invalid return type: {msg}" | ||
| )))), | ||
| } | ||
| } else { | ||
| ResumeAction::ReturnObj(obj) | ||
| } | ||
| } | ||
| ExternalResult::Error(exc) => { | ||
| let err = RunError::from(exc); | ||
| ResumeAction::Error(err) | ||
| } | ||
| ExternalResult::Future => ResumeAction::Future, | ||
| }; |
There was a problem hiding this comment.
this is ugly, we should add Datetime, Date etc. varients to MontyObject
There was a problem hiding this comment.
I guess these variants should use whatever Datetime and Date etc. struct we settle on - e.g. our own Datetime type that wraps a chrono or speedate date.
crates/monty/src/types/date.rs
Outdated
|
|
||
| /// Creates a date from a proleptic Gregorian ordinal value. | ||
| pub(crate) fn from_ordinal(ordinal: i32) -> RunResult<Date> { | ||
| let days_since_epoch = i64::from(ordinal) - 719_163; |
There was a problem hiding this comment.
where is 719_163 coming from, we need more explanation
There was a problem hiding this comment.
there's a lot of logic here. I know I recommended speedate, but would this be simpler if we used chrono?
I initially suggested not to use chrono because of it's leap second stuff, but maybe it would be cleaner than this???
crates/monty/src/types/date.rs
Outdated
| }; | ||
|
|
||
| /// `datetime.date` storage backed directly by `speedate`. | ||
| pub(crate) type Date = speedate::Date; |
There was a problem hiding this comment.
you should wrapp the speedate Date (or chrono date) like
pub(crate) struct Date(speedate::Date);
Then you can implement (de)serialization directly here.
crates/monty/src/types/datetime.rs
Outdated
| if !(1..=9999).contains(&year) { | ||
| return Err(SimpleException::new_msg(ExcType::ValueError, format!("year {year} is out of range")).into()); | ||
| } | ||
| if !(1..=12).contains(&month) { | ||
| return Err(SimpleException::new_msg(ExcType::ValueError, "month must be in 1..12").into()); | ||
| } |
There was a problem hiding this comment.
you've missed days, but also whatever logic we use here should be shared with Date.
421cfb7 to
5c87707
Compare
|
@samuelcolvin i think i got to all the feedback -- i switched to chrono which let us drop a lot of the custom logic around date / datetime. i'm not sure how to handle the vendored typeshed. i previously had bumped that and it brought in all the other changes. the failing test is because datetime isn't in the stdlib. |
f1a42f8 to
2ce1535
Compare
…resolution - Extracted magic numbers like 86_400, 1_000_000 into constants in timedelta, timezone, date, datetime and convert modules - Replaced invalid const 'i128::from' in timedelta constants and fixed subsec_nanos division to prevent compilation errors - Added manual Hash implementation for DateTime based on aware/naive py_eq equivalence rather than raw fields - Updated typeshed to use upstream datetime.pyi directly instead of relying on custom stub overrides and build.rs zip manipulations
- Added HeapDataMut variants for datetime types so VM generic paths don't panic on to_mut - Fixed Value::py_cmp fallback to dispatch to HeapData::py_cmp instead of returning None - Adjusted Option<Ordering> evaluation for None (Float NaN) vs TypeError (incomparable types)
…rooting - Updated heap GC mark phase to keep the lazily-allocated datetime.timezone.utc singleton alive - Updated MontyObject -> HeapData::TimeZone conversion to reuse the cached UTC singleton instead of allocating new instances, maintaining 'is' identity - Rejected explicit None passed as the name to timezone() constructors to match CPython's TypeError behavior
0ccc76d to
fecf84f
Compare
Scope
This PR is intentionally kept narrow to make review tractable.
Deferred From This PR
These changes were removed from the diff against
mainand will land in a follow-up PR:crates/monty-typeshed/vendor/typeshed/**crates/monty/src/bytecode/vm/async_exec.rscrates/monty/src/bytecode/vm/binary.rscrates/monty/src/bytecode/vm/call.rscrates/monty/src/bytecode/vm/compare.rscrates/monty/src/bytecode/vm/mod.rscrates/monty/src/bytecode/vm/scheduler.rscrates/monty/src/types/module.rsDependent cleanups were also reverted to keep the branch compiling with the rollback:
crates/monty/src/repl.rscrates/monty/src/run_progress.rsWhat Those Deferred VM/Module Changes Were Trying To Do
datetime.nowOS calls so async future resolution can reconstructdate.today(), naivedatetime.now(), and fixed-offset awaredatetime.now(tz=...)correctly.datetime.date/datetime.datetime) handle class methods consistently.Deferred Regression Coverage
The intended failing cases are captured as commented tests in:
crates/monty/test_cases/datetime__core.pyThose snippets are intentionally commented out in this PR and should be re-enabled in the follow-up PR that restores the deferred VM/module work.