[5895] added code to raise error in sleep if invalid value#5907
[5895] added code to raise error in sleep if invalid value#5907DominicPrince2003 wants to merge 3 commits into
Conversation
Sorry, something went wrong.
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant time_sleep
participant VirtualMachine
Caller->>time_sleep: sleep(secs: PyObjectRef, vm)
time_sleep->>time_sleep: Convert secs to f64 or i64
alt Conversion fails
time_sleep->>VirtualMachine: Raise TypeError
else Conversion succeeds
time_sleep->>time_sleep: Validate value (finite, non-negative)
alt Validation fails
time_sleep->>VirtualMachine: Raise ValueError
else Validation passes
time_sleep->>time_sleep: Convert to Duration
time_sleep->>time_sleep: Thread sleep for Duration
alt Unix platform and interrupted
time_sleep->>VirtualMachine: vm.check_signals()
end
time_sleep-->>Caller: Ok(())
end
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @DominicPrince2003, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the robustness of the time.sleep() function by implementing comprehensive input validation. It ensures that the duration argument is a valid, finite, non-negative number, raising specific Python exceptions for invalid inputs. This change improves the reliability and predictability of the sleep function's behavior.
Highlights
- Input Validation: The
sleep()function now performs robust input validation. It ensures that the provided duration is a valid number (float or integer) and falls within an acceptable range (non-negative, finite, and not exceedingu64::MAX). - Error Handling: Appropriate
TypeErrorandValueErrorexceptions are raised whensleep()receives invalid input, aligning its behavior with standard Python error handling for such cases. - Cross-platform Consistency: The new validation logic has been applied to both the generic
sleepimplementation (for non-Unix systems) and the platform-specificsleepimplementation (likely for Unix systems usinglibc::nanosleep), ensuring consistent behavior across different environments.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request adds input validation to time.sleep() to handle non-numeric, negative, and non-finite values, which is a great improvement.
My review includes a few points:
- There's some code duplication between the
unixandnon-uniximplementations ofsleep. Extracting the validation logic to a helper function would improve maintainability. - The
uniximplementation has a minor indentation issue. - The upper bound check for the sleep duration on
non-unixplatforms might not be accurate for Windows, potentially leading to silent truncation of the sleep time.
Overall, good work on improving the robustness of this function.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_time.pyis excluded by!Lib/**
📒 Files selected for processing (1)
vm/src/stdlib/time.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/stdlib/time.rs
🧠 Learnings (1)
vm/src/stdlib/time.rs (3)
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
PR: RustPython/RustPython#5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
🔇 Additional comments (2)
vm/src/stdlib/time.rs (2)
544-544: LGTM: Import addition is necessary.The
TryFromObjectimport is required for the new sleep function implementation that converts Python objects to numeric types.
95-101: Good implementation of Python object conversion.The two-stage conversion approach (f64 first, then i64) is well-designed as it preserves fractional seconds while still accepting integer inputs. This matches Python's expected behavior.
Also applies to: 711-717
Sorry, something went wrong.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
Unfortunately, since #5906 was submitted earlier and addresses the same issue, I decided to merge that patch first.
However, your addition of a test for negative floats is still valuable, as we currently don't have coverage for that case.
I'd still like to merge the test you wrote as an additional improvement to the project.
Sorry, something went wrong.
Reviewers
+1 more reviewer
Requested changes must be addressed to merge this pull request.
2 participants
fixes #5895. I have added code to validate the input in the sleep() and raise the required errors. modified the test file and added few more test cases. Please let me know if any further changes are required.
Summary by CodeRabbit
New Features
sleepfunction to accept both integer and floating-point values for seconds, improving flexibility in specifying sleep durations.Bug Fixes