feat(type): extend type builtin to search for executables#5
Conversation
📝 WalkthroughWalkthroughThe "type" builtin now searches PATH for non-builtin commands: it reads PATH, iterates directories, checks file existence and executable permissions, prints the resolved path if found, or reports not found (with an error message if PATH cannot be read). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Shell
participant Env as PATH env
participant FS as Filesystem
User->>Shell: run `type <query>`
Shell->>Shell: check builtin list
alt builtin found
Shell->>User: print builtin info
else not builtin
Shell->>Env: read PATH
Env-->>Shell: PATH string or error
alt PATH readable
Shell->>FS: iterate directories -> check path exists
alt file exists
Shell->>FS: fs::metadata & permissions check (0o111)
alt executable
Shell->>User: print resolved full path
else not executable
Shell->>FS: continue next directory
end
else not exists
Shell->>FS: continue next directory
end
opt none found
Shell->>User: "<query>: not found"
end
else PATH read error
Shell->>User: print PATH read error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| use std::env; | ||
| use std::path::Path; | ||
| use std::fs; | ||
| use std::os::unix::fs::PermissionsExt; |
There was a problem hiding this comment.
Consider conditional compilation for platform-specific code.
The PermissionsExt trait (line 5) and the PATH separator ":" (line 42) are Unix-specific. The code won't compile on Windows without conditional compilation.
If Unix-only support is intentional, consider documenting this or using #[cfg(unix)] to make it explicit. Otherwise, add Windows support with conditional compilation.
🔎 Example conditional compilation approach
use std::io::{self, Write};
use std::env;
use std::path::Path;
use std::fs;
+#[cfg(unix)]
use std::os::unix::fs::PermissionsExt;And in the PATH splitting logic:
#[cfg(unix)]
let paths: Vec<&str> = val.split(":").collect();
#[cfg(windows)]
let paths: Vec<&str> = val.split(";").collect();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
please create a issue for this will take care in future
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the platform-specific code concern
gh issue create \
--repo Pawank06/rshell \
--title "Add cross-platform support for type builtin" \
--body "## Context
The \`type\` builtin currently uses Unix-specific code that won't compile on Windows:
- \`std::os::unix::fs::PermissionsExt\` trait (line 5 in src/main.rs)
- PATH separator \":\" is Unix-specific (Windows uses \";\")
## Suggested approach
Consider using conditional compilation with \`#[cfg(unix)]\` and \`#[cfg(windows)]\` attributes to support both platforms, or document that the shell is Unix-only.
## References
- PR: https://github.com/Pawank06/rshell/pull/5
- Comment: https://github.com/Pawank06/rshell/pull/5#discussion_r2658099015
- Requested by: @Pawank06" \
--assignee Pawank06
Length of output: 43
@Pawank06, I've created issue #6 to track this: #6
The issue is assigned to you and includes all the context about adding cross-platform support for the type builtin. Feel free to tackle it whenever you're ready! 👍
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main.rs (2)
2-5: Platform-specific code without conditional compilation.The
PermissionsExttrait (line 5) and the PATH separator ":" (line 42) remain Unix-specific. As noted in the previous review, this code won't compile or run correctly on Windows without conditional compilation using#[cfg(unix)]and#[cfg(windows)].
68-68: Improve error message clarity.The error message "Couldn't find val {}" is unclear. It doesn't indicate that the failure is related to reading the PATH environment variable.
🔎 Proposed fix for clearer error message
- Err(e) => println!("Couldn't find val {}", e) + Err(e) => println!("Failed to read PATH environment variable: {}", e)
🧹 Nitpick comments (1)
src/main.rs (1)
46-48: Consider removing redundant exists() check.The
exists()check is redundant sincefs::metadata()at lines 50-53 will fail for non-existent files anyway. Removing it would simplify the logic without affecting correctness.🔎 Optional simplification
for dir in paths { let full_path = Path::new(dir).join(query); - if !full_path.exists() { - continue; - } let metadata = match fs::metadata(&full_path) { Ok(m) => m, Err(_) => continue };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.rs
🔇 Additional comments (1)
src/main.rs (1)
40-69: LGTM! Executable permission check has been corrected.The PATH lookup implementation is now correct. Line 57 properly uses
mode & 0o111to check for any execute permission (owner, group, or other), fixing the critical issue flagged in the previous review wheremode & 0o11was incorrectly used.The logic correctly:
- Reads and splits PATH by directories
- Builds full paths and checks for existence
- Validates executable permissions
- Reports found executables or "not found"
1 participant
summary
notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.