Implementation for text_grep - Issue (Searcher (rustcore) #1715) #1986
Conversation
| process_file(&file_path, &patterns, chunk_size, &cancel_token, &sender) | ||
| { | ||
| if error_sender_clone.send(err.to_string()).is_err() { | ||
| eprintln!("Error sending error message through channel"); |
There was a problem hiding this comment.
crate should not post any to console, it should return error in case of error
| let error_sender_clone = error_sender.clone(); | ||
| let cancel_token = cancel_token_clone.clone(); | ||
| let file_path = Arc::clone(file_path); | ||
| thread::spawn(move || { |
There was a problem hiding this comment.
why we need to spawn system thread with async method? In general it's very expensive. If we really need threads here tokio's task should be used. But I don't think we need it at all.
| file_path: &Arc<str>, | ||
| patterns: &[Arc<str>], | ||
| chunk_size: usize, | ||
| cancel_token: &CancellationToken, |
There was a problem hiding this comment.
Using Arc potentially will give more headaches than cloning.
| chunk_size: usize, | ||
| cancel_token: &CancellationToken, | ||
| sender: &mpsc::Sender<Result<SearchResult, String>>, | ||
| ) -> Result<(), String> { |
There was a problem hiding this comment.
Crate should have own Error enum. Please take a look, for example, to crate thiserror, which helps to create it. See examples in other places of chipmunk's solution.
| if !is_text_file(&file_path) { | ||
| let error_msg = format!("File '{}' is not a text file", file_path.display()); | ||
| if sender.send(Err(error_msg.clone())).is_err() { | ||
| eprintln!("Error sending search result through channel"); |
There was a problem hiding this comment.
no output into terminal from crate
| .collect(); | ||
|
|
||
| for handle in thread_handles { | ||
| handle.join().unwrap(); |
There was a problem hiding this comment.
unwrap() is acceptable only in tests and very-very specific use-cases. Please, check for error here and return some error if it is. For ex: handle.join().map_err(|e| e.into())?;
|
@itsmesamster thanks for PR. I have a couple of comments in general:
|
|
Some random measurements and proof that the count is correct as per ripgrep, depending on the case sensitivity flag. |
itsmesamster
force-pushed
the
test
branch
2 times, most recently
from
aad7834 to
cc45ecb
Compare
Working code for text_grep - Issue Searcher (rustcore) esrlabs#1715
…unt of pattern matches
No description provided.