[cron] full implementation #382
Conversation
|
@jgarzik please, review, sir |
There was a problem hiding this comment.
Pull Request Overview
This PR implements full cron functionality including command-line binaries, a cron job scheduler library, and comprehensive tests for both crontab and crond.
- Added integration tests for crontab and crond commands
- Implemented the cron job parsing, scheduling, and execution logic in the job module
- Updated the Cargo workspace configuration to include the new cron package
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cron/tests/crontab_test.rs | Added tests ensuring crontab command returns error status |
| cron/tests/crond_test.rs | Added tests for crond command, including edge cases |
| cron/src/main.rs | Minimal main function for the cron package |
| cron/src/lib.rs | Exports the job module |
| cron/src/job.rs | Implements cron job parsing, scheduling, and execution |
| cron/Cargo.toml | Configures binaries and the library for the cron package |
| Cargo.toml | Updates workspace settings to include cron |
Comments suppressed due to low confidence (1)
cron/src/job.rs:223
- [nitpick] Remove the debug print statement in next_execution to avoid exposing internal state in production.
println!("{:?}", monthdays.to_vec());
cron/src/job.rs
Outdated
|
|
||
| fn merge(self, other: Self) -> Self { | ||
| let mut a = self.to_vec(); | ||
| let b = self.to_vec(); |
There was a problem hiding this comment.
The merge function is incorrectly using self.to_vec() twice; replace the second occurrence with other.to_vec() to merge distinct sets.
| let b = self.to_vec(); | |
| let b = other.to_vec(); |
Copilot uses AI. Check for mistakes.
cron/src/job.rs
Outdated
| .iter() | ||
| .filter(|x| x.next_execution(&now).is_some()) | ||
| .min_by_key(|x| { | ||
| println!("{x:?}"); |
There was a problem hiding this comment.
[nitpick] There is an unintended debug print in nearest_job that may clutter production logs; consider removing it.
| println!("{x:?}"); |
Copilot uses AI. Check for mistakes.
cron/Cargo.toml
Outdated
| path = "src/bin/crond.rs" | ||
|
|
||
| [[bin]] | ||
| name = "main" |
There was a problem hiding this comment.
There should not be a binary called "main", just crontab and crond
cron/src/bin/crond.rs
Outdated
| .fold(Database(vec![]), |acc, next| acc.merge(next))) | ||
| } | ||
|
|
||
| fn main() -> Result<(), Box<dyn std::error::Error>> { |
|
@jgarzik fixed. Let me know if there is anything else you want to improve? |
No description provided.