feat: support except file comand line argument #9
Conversation
![mentatbot[bot]](https://avatars.githubusercontent.com/in/837875?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've reviewed the changes for the random pairs of s3file tool. Overall, the implementation looks good with proper error handling and documentation. Here are a few suggestions for improvement:
Other than these minor suggestions, the implementation looks solid with:
- ✅ Proper error handling for S3 operations
- ✅ Efficient use of HashSet for exclusions
- ✅ Clear command line argument documentation
- ✅ Maintainable and well-structured code
The feature will be useful for users who need to exclude certain files from the random pairing process.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
You can change my review settings on my settings page.
| @@ -50,6 +57,14 @@ async fn main() -> Result<(), Box<dyn Error>> { | |||
| let directory_prefix = &args.directory; | |||
| let url_prefix = &args.url_prefix; | |||
|
|
|||
| // Read excluded keys from file if provided | |||
| let excluded_keys: HashSet<String> = if let Some(exclude_file_path) = args.exclude_file { | |||
There was a problem hiding this comment.
Consider adding error handling for individual lines in the exclude file. Currently, invalid lines are silently skipped with map_while(Result::ok). It might be better to either:
- Log warnings for invalid lines
- Return an error if any line is invalid
- Add documentation explaining that invalid lines are skipped
| @@ -77,6 +92,7 @@ async fn main() -> Result<(), Box<dyn Error>> { | |||
| let all_keys: Vec<String> = objects | |||
There was a problem hiding this comment.
Consider adding a debug log or print statement showing how many keys were excluded. This would help users verify that their exclude file is working as expected.
|
|
||
| ```shell | ||
| Usage: random_pairs_of_s3file [OPTIONS] --num-pairs <NUM> --bucket <BUCKET> --directory <DIR> --url-prefix <PREFIX> | ||
| Usage: random_pairs_of_s3file [OPTIONS] --num-pairs <NUM_PAIRS> --bucket <BUCKET> --directory <DIRECTORY> --url-prefix <URL_PREFIX> |
There was a problem hiding this comment.
The README documentation is good, but it would be helpful to add:
- An example of the exclude file format
- Whether the exclude file should contain full S3 keys or just filenames
- A complete example command with the exclude file option
sangshuduo
force-pushed
the
feat/sangshuduo/random-pairs-with-s3
branch
from
7740d0a to
536f4fb
Compare
No description provided.