Skip to content

Fix time.strptime #6208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
youknowone merged 1 commit into from
Oct 23, 2025
Merged

Fix time.strptime #6208

youknowone merged 1 commit into from
Oct 23, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Oct 23, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • Bug Fixes
    • Updated time.strptime() to use the standard CPython parsing behavior — error responses and returned time representations now match CPython for improved compatibility and more consistent parsing results.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025 •
edited
Loading

Walkthrough

The strptime function in the time module was changed to delegate parsing to CPython's _strptime._strptime_time (imported at runtime) instead of using local NaiveDateTime::parse_from_str. The function signature was generalized from PyResult<PyStructTime> to PyResult.

Changes

Cohort / File(s) Summary
Time module strptime refactor
vm/src/stdlib/time.rs		 Replaced in-file parsing with delegation: import _strptime and call its _strptime_time for parsing. Updated signature from fn strptime(...) -> PyResult<PyStructTime> to fn strptime(...) -> PyResult. Error handling now propagates the delegated call's result.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant strptime as strptime()
    participant _strptime as _strptime module

    rect rgb(230, 245, 255)
    Note over strptime,_strptime: Delegation to CPython-style implementation
    Caller->>strptime: strptime(string, format?)
    strptime->>_strptime: import _strptime and call _strptime_time(string, format)
    _strptime-->>strptime: PyResult (success or error)
    strptime-->>Caller: PyResult
    end

    rect rgb(245, 230, 255)
    Note over strptime: Previous local parse via NaiveDateTime::parse_from_str (removed)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hopping through the code I go,
I hand the clocks to Python's flow,
No more parsing in my den,
I call the old strptime then,
A lighter hop, and off I go! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Fix time.strptime" is directly and clearly related to the primary change in the changeset, which modifies the strptime function in vm/src/stdlib/time.rs. The title specifically identifies the target component (time.strptime) and indicates the nature of the change (a fix). While the title does not detail the specific implementation changes (delegation to CPython-style _strptime_time and signature modifications), it provides sufficient clarity for a teammate scanning the commit history to understand that this pull request addresses improvements or corrections to the strptime function. The title is concise, readable, and avoids vague or generic terminology.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73146cc and 34c56f8.

⛔ Files ignored due to path filters (2)
  • Lib/test/test_time.py is excluded by !Lib/**
  • Lib/test/test_zipfile.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • vm/src/stdlib/time.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • vm/src/stdlib/time.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Ensure compilation on various targets

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone merged commit 2463bdf into RustPython:main Oct 23, 2025
12 checks passed
@youknowone youknowone deleted the time-strptime branch October 23, 2025 08:28
@coderabbitai coderabbitai bot mentioned this pull request Dec 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone