feat: Add multi-provider stock website support #34
Conversation
meta-cla
bot
added
the
CLA Signed
alichtman
left a comment
alichtman
left a comment
There was a problem hiding this comment.
This is a great start! A few comments on things that stood out to me.
|
appreciate the review! i'll address these in a few days, just leaving this comment to let you know i'm not leaving this stale |
|
@alichtman thanks for all the reviewing and suggestions! looks a lot cleaner this way. I apologize for being slow with the iteration.. please review when you have the time :) |
| log_level = "{}" | ||
| {} | ||
| "#, | ||
| if let Some(browser) = &self.browser { |
There was a problem hiding this comment.
I regret not extracting this logic to a variable -- it's kinda hard to read. I'll fix that in a future commit
| @@ -23,6 +38,12 @@ | |||
| #[serde(default = "default_search_engine")] | |||
There was a problem hiding this comment.
This should probably be default = "google" and stock_provider below should probably be default = "yahoo", and non-optional
| let encoded_query = | ||
| percent_encoding::utf8_percent_encode(query, percent_encoding::NON_ALPHANUMERIC) | ||
| .to_string(); | ||
| let encoded_query = encode_url_special_char(query); |
There was a problem hiding this comment.
IN retrospect, this function probably should have been internal to some kind of search command / module, instead of inlined in the config file
|
Thanks for putting this PR together! |
2 participants
#32 adds support for multiple stock website providers!
feat details:
testing:
+manual testingnitpicks are welcome too, just starting to learn with rustlings and figured ill jump into a open source repo :) any feedback is appreciated!