Skip to content

impl more ntpath #6531

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
Dec 26, 2025
Merged

impl more ntpath #6531

youknowone merged 1 commit into from
Dec 26, 2025

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Dec 25, 2025 •
edited by coderabbitai bot
Loading

Summary by CodeRabbit

  • New Features

    • Added capability to detect whether a file path is located on a Windows Dev Drive, providing developers with the ability to identify and handle Dev Drive volumes.

  • Refactor

    • Enhanced string method implementation with improved handling of different encoding types, ensuring more consistent and reliable behavior across various text processing scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 25, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes modify the casefold method in PyStr to return Self instead of String with branching logic for different string kinds (Ascii, Utf8, Wtf8), and add a new _path_isdevdrive function to the nt module that detects Windows Dev Drives via sequential Windows API calls.

Changes

Cohort / File(s) Summary
PyStr casefold refactoring
crates/vm/src/builtins/str.rs		 Updated casefold method signature from returning String to Self. Implemented branching logic based on string kind: Ascii and Utf8 apply default_case_fold_str directly, while Wtf8 processes chunks separately, preserving surrogates.
Windows Dev Drive detection
crates/vm/src/stdlib/nt.rs		 Added _path_isdevdrive(path, vm) function. Queries Windows API (GetVolumePathNameW, GetDriveTypeW, CreateFileW, DeviceIoControl) to determine if a path resides on a Windows Dev Drive. Introduced FILE_FS_PERSISTENT_VOLUME_INFORMATION structure and PERSISTENT_VOLUME_STATE_DEV_VOLUME constant.

Sequence Diagram

sequenceDiagram
    participant Code as Caller Code
    participant API as Windows API
    participant Volume as Volume State
    
    Code->>API: GetVolumePathNameW(path)
    API-->>Code: volume_path
    
    Code->>API: GetDriveTypeW(volume_path)
    API-->>Code: drive_type
    
    alt drive_type == DRIVE_FIXED
        Code->>API: CreateFileW(volume_path, OPEN_EXISTING)
        API-->>Code: volume_handle
        
        Code->>API: DeviceIoControl(handle, FSCTL_QUERY_PERSISTENT_VOLUME_STATE)
        API-->>Volume: Query state
        Volume-->>API: PERSISTENT_VOLUME_STATE
        API-->>Code: volume_state
        
        Code->>Code: Check PERSISTENT_VOLUME_STATE_DEV_VOLUME flag
        Code->>API: CloseHandle(volume_handle)
        API-->>Code: success
    else Not DRIVE_FIXED
        Code-->>Code: Return false
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Strings now fold back onto themselves, swift and sure,
While Windows paths reveal their Dev Drive nature,
Branching logic blooms across the Utf8 garden,
APIs dance in sequence—no need to beg pardon!
Modest changes, mighty purpose—carrots of code! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'impl more ntpath' is vague and does not clearly convey what was implemented. It uses generic phrasing that lacks specificity about the actual changes. Use a more descriptive title that specifies what was added or changed, such as 'Add _path_isdevdrive to ntpath module' or 'Implement casefold return type changes and add Dev Drive detection'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
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 2063c1e and 5c02ff2.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_ntpath.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/vm/src/builtins/str.rs
  • crates/vm/src/stdlib/nt.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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

Files:

  • crates/vm/src/builtins/str.rs
  • crates/vm/src/stdlib/nt.rs
🔇 Additional comments (6)
crates/vm/src/stdlib/nt.rs (5)

594-608: LGTM! Volume resolution logic is correct.

The function properly resolves the volume path using GetVolumePathNameW and efficiently filters out non-fixed drives before making the more expensive IOCTL call.

610-624: LGTM! Handle creation uses appropriate flags.

The CreateFileW call correctly uses FILE_READ_ATTRIBUTES (minimal required access) and FILE_FLAG_BACKUP_SEMANTICS (required for opening volumes), with proper error handling.

626-647: LGTM! IOCTL call and resource management are correct.

The DeviceIoControl call properly uses the same structure for both input and output buffers (standard pattern for FSCTL_QUERY_PERSISTENT_VOLUME_STATE), and the handle is correctly closed immediately after the call, even before checking the result.

649-659: LGTM! Error handling gracefully handles unsupported systems.

The special handling of ERROR_INVALID_PARAMETER (returning Ok(false) instead of raising an error) is correct, as it indicates the Dev Drive feature is not supported on the current Windows version. The bitwise flag check is also correct.

582-592: The FileFsPersistentVolumeInformation structure definition and PERSISTENT_VOLUME_STATE_DEV_VOLUME constant (0x00002000) are correct and match the Windows SDK specification. The structure layout, field types, and constant value are all accurate.

crates/vm/src/builtins/str.rs (1)

670-684: LGTM. The implementation correctly handles case folding across all string kinds (Ascii, Utf8, Wtf8) and properly preserves surrogates in Wtf8 chunks. The return type change from String to Self improves API consistency with other string transformation methods. The caseless crate (0.2.2) is the latest stable version.

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 marked this pull request as ready for review December 25, 2025 23:16
@youknowone youknowone merged commit ae39b13 into RustPython:main Dec 26, 2025
13 checks passed
@youknowone youknowone deleted the ntpath branch December 26, 2025 00:43
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