feat(data): fetch stocks data from Alpha Vantage #65
Conversation
* refactor: string formatting * refactor: update string formatting
Signed-off-by: Carlo Bortolan <106114526+carlobortolan@users.noreply.github.com>
|
Hey @carlobortolan, I configured my commits to be signed and also ensured I passed the |
|
@SaurabhJamadagni Thanks! I'll take a look at the changes over the weekend; one idea regarding the signed commits: have you uploaded your keys to github? https://github.com/settings/keys |
There was a problem hiding this comment.
@SaurabhJamadagni: Very nice - thanks for the good work so far! I've added some small comments.
I also wanted to ask your opinion regarding the naming of some things:
traits::StocksSource->traits::QuoteProvider(stock_source is already the name of the module)GlobalQuote->Quote(GlobalQuote might be too specific to Alpha Vantage?)
| example_data_module().await; | ||
| } | ||
|
|
||
| async fn example_data_module() { |
There was a problem hiding this comment.
maybe put this in a separate file inside the examples/ directory
| self.base_url, symbol, self.api_key | ||
| ); | ||
|
|
||
| let response = reqwest::get(&url).await.unwrap(); |
There was a problem hiding this comment.
You're using .unwrap(); if the network call fails (e.g., DNS error, timeout), the program will panic. Something like this should fix this:
let response = reqwest::get(&url).await.map_err(|e| {
Error::new(std::io::ErrorKind::Other, format!("Request failed: {}", e))
})?;There was a problem hiding this comment.
Yes for sure! I was just really wanting to see some output so took a bit of a lousy approach to exception handling. I'll factor that in.
There was a problem hiding this comment.
You’re using reqwest::get() directly, which creates a new HTTP client on every request.
I'd suggest instantiating it once and then reusing it:
pub struct AlphaVantageSource {
client: reqwest::Client,
base_url: String,
api_key: String,
}
impl AlphaVantageSource {
pub fn new(user_key: &str) -> Self {
AlphaVantageSource {
client: reqwest::Client::new(),
base_url: "https://www.alphavantage.co/query".to_string(),
api_key: user_key.to_string(),
}
}
}
...
let response = self.client.get(&url).send().await.map_err(...)?
For this, I was assuming anything related to a stock ticker would fall under this trait. So fundamentals data as well. Or were you thinking fundamentals data would be a different trait?
Agree on this. I'll change that right away. Thanks for all your feedback @carlobortolan :) |
@SaurabhJamadagni ah, got it; I was thinking that |
Oh I see what you mean too. I didn't think about having multiple traits going in the same module. But that sounds good, I agree with you. Thanks @carlobortolan! On it! |
PR in reference to issue
PR changes include:
Todo before final PR:
This PR is currently in a draft stage. PR description to be updated with progressing changes.
@carlobortolan please let me know your feedback on the flow. I'll extend the functionality with your feedback in mind. Thanks!