This looks good! I just have some random comments.
Some minor forum usage comments: I noticed this post's category is "tutorial" and I think it should be "code review". And I saw you selected a solution for this, which is normally only done for "help" posts. When someone replies to a post with a selected solution, a warning will appear saying that it has already been solved. I assume you want more feedback on this, and if so I would uncheck the solution.
if let Some(captures) = re_start.captures(&normal_text) {
if let Some(main_py_code) = captures.get(1) {
return Some(main_py_code.as_str().to_string());
}
} else {
println!("No match found");
}
None
Above, this won't always print a message when returning None. Was it your intention to only print a message when the first condition fails, but not when the second condition fails? I'm wondering if this is accidental.
In a couple places I noticed that there is unnecessary String cloning, which is not a problem at all when learning Rust and when performance is not critical. However, I'll point these out just to help expand your knowledge of Rust.
The first is that MessageContent has a blanket From impl saying it will convert from any type that has an Into<String>
impl. This includes &str
since it does have an Into<String>
impl, which is the type of the field you're converting into a String. Therefore, you can just pass the &str
values. Again, this is not a problem at all, but can be simplified as follows:
- ChatMessage::system(&self.prompt_text.to_owned()),
- ChatMessage::user(&self.user_text.to_owned())
+ ChatMessage::system(self.prompt_text),
+ ChatMessage::user(self.user_text),
Another thing that caught my eye is this cloning of the text
param:
fn extracted_text(&self, text: String, file_name: &str) -> Option<String> {
let normal_text = text.clone();
This makes me wonder if you have a misunderstanding of owned values. The type String
is not a reference, so the function owns it when it is passed. Therefore it only needs to be cloned if the function needs two copies of it, and it does not. So the clone can be removed.
I'll try to post more comments later.
Another unnecessary String clone is caused by passing the text
param mentioned above as a String when only &str
is needed. The param type can be changed to &str
(in the interface as well) to avoid the clone. This plus the prior change will avoid two String clones:
- fn extracted_text(&self, text: String, file_name: &str) -> Option<String>;
+ fn extracted_text(&self, text: &str, file_name: &str) -> Option<String>;
- let item = self.extracted_text(text.clone(), file_name).unwrap();
+ let item = self.extracted_text(&text, file_name).unwrap();
- fn extracted_text(&self, text: String, file_name: &str) -> Option<String> {
- let normal_text = text.clone();
+ fn extracted_text(&self, text: &str, file_name: &str) -> Option<String> {
- if let Some(captures) = re_start.captures(&normal_text) {
+ if let Some(captures) = re_start.captures(text) {
It is common to tell people learning Rust to clone as much as they need to when trying to fix compiler errors, since this makes learning easier. OTOH, in the cases above there is no need to clone and you could say the parameter types were incorrect. When a parameter value is only used by the function temporarily -- it is not consumed and ownership is not needed -- then a reference (borrow) should be used. In addition to being efficient, this helps in understanding the code, and it gives more flexibility to the caller of the function since the caller does not have to give up ownership or consider the cost of cloning.