Latest trade data #43

sbeam · 2022-10-24T19:42:07Z

This uses the Latest Multi Trades endpoint to get the latest trade data for one or more symbols at once. This was heavily copied from the last_quote mod, but changed to deal with the JSON response as nested objects. I elected to return a vec of Trades with the symbol attached because it suits my needs. closes #42

d-e-s-o · 2022-10-25T15:43:55Z

Nice! Thanks! I will take a closer look later this week, but one quick thought: Instead of copying the last_quote module to last_trade and then modifying it to work with multiple symbols, would you be up for modifying last_quote to gain multi symbol support first? In my opnion it's more universal, so I don't see a downside. Last I checked I recall that there was a similar multi symbol endpoint there.

sbeam · 2022-10-25T18:40:57Z

yes that would make sense, they are similar and both have a single symbol endpoint and multi. However the last_quote currently returns a single Quote, and changing it to a Vec would be breaking. Maybe would be best to have last_quote, last_quote_multi, last_trade and last_trade_multi? So all is nice and consistent?

d-e-s-o · 2022-10-26T02:00:50Z

yes that would make sense, they are similar and both have a single symbol endpoint and multi. However the last_quote currently returns a single Quote, and changing it to a Vec would be breaking.

But to be honest, backwards compatibility is not much of a concern for me at this point. The API surface is too large to nail everything early on, enabling more without a need is a maintenance burden, and stuff is changing upstream. So don't worry about it, we just bump the crate's version with the next release.

Maybe would be best to have last_quote, last_quote_multi, last_trade and last_trade_multi? So all is nice and consistent?

It's an option, yeah. From my perspective, though, one is basically a superset of the other and so we may as well opt for the fewer-lines-of-code option and just stick with the multi versions of both.

sbeam · 2022-10-31T15:17:10Z

got it. I'll work on this at some point this week

sbeam · 2022-11-03T14:23:46Z

@d-e-s-o ok, see what you think. I wanted to use this library in my project to learn Rust and it's been very helpful in that regard! To reduce the copy/paste factor, chose to implement a couple generic traits to pull the data objects out of the json -- maybe not worth the trouble but it works. Also added examples etc

d-e-s-o

Sorry for the delay.

@d-e-s-o ok, see what you think. I wanted to use this library in my project to learn Rust and it's been very helpful in that regard! To reduce the copy/paste factor, chose to implement a couple generic traits to pull the data objects out of the json -- maybe not worth the trouble but it works. Also added examples etc

Yeah, I'd say let's not add the trait. It doesn't really add value from my perspective and just complicates things. I'd also suggest we keep the pull request concentrated on the plan we laid out earlier.

d-e-s-o · 2022-11-08T05:05:59Z

Cargo.toml

+[[example]]
+name = "order"
+
+[[example]]
+name = "stream-realtime-data"
+


Can you please create a separate commit for these changes?

d-e-s-o · 2022-11-08T05:11:24Z

src/data/v2/mod.rs

@@ -8,9 +8,14 @@ mod unfold;
 pub mod bars;
 /// Functionality for retrieval of the most recent quote.
 pub mod last_quote;
+/// Functionality for retrieval of the most recent trade(s).
+pub mod latest_trade;


Why are we calling one "last" and the other "latest"? Seems inconsistent.

d-e-s-o · 2022-11-08T05:13:40Z

src/data/v2/last_quote.rs

+    let api_info = ApiInfo::from_env().unwrap();
+    let client = Client::new(api_info);
+
+    let req = LastQuoteReq::new("SPY,QQQ,MSFT");


I don't think we should be passing in some kind of pre-formatted string. It should likely be a vec/iter/slice/whatever of strings. If I recall correctly there is precedent in other parts of the API surface that we should follow, unless not suitable for some reason.

it's what alpaca wants, but I agree it's a bit icky. will fix

Add support for Latest Multi Trades endpoint to get the latest trade data for one or more symbols in one call. For consistency, also converted the existing latest_quote mod to also fetch and represent multiple symbols at once.

sbeam · 2022-11-15T19:40:57Z

@d-e-s-o ok, updated as requested and squashed

d-e-s-o · 2022-11-17T05:41:12Z

@d-e-s-o ok, updated as requested and squashed

Thank you! Will have reviewed by the end of the weekend the latest.

d-e-s-o

I won't be able to land this code without major modifications and I don't have the time to write 2+ hour reviews for each iteration, which is pretty much guaranteed to annoy the both of us.

My suggestion is that you create a pull request for the changes to the last_quote module first (renaming it to last_quotes in the process and addressing the comments I left there). We get that in. Then we work on the rest.

I expect you to understand existing code and why it does what it does or at least accept that it is there for a reason and not change it randomly, though, please. The general guidance for open source contributions to be accepted is to mirror existing project patterns, styles, etc. I am happy to explain/fix minor things, but I don't have the time or nerves to rework changes that make no attempt at blending in or doing research on existing patterns.

Edit: Of course also no hard feelings if you don't want to embark on this process or disagree with my stance.

d-e-s-o · 2022-11-20T19:18:41Z

src/data/v2/last_quote.rs

-/// A helper for initializing [`LastQuoteReq`] objects.
-#[derive(Clone, Debug, Default, Eq, PartialEq)]
-#[allow(missing_copy_implementations)]
-pub struct LastQuoteReqInit {


I don't see a good reason why this type is being removed?

d-e-s-o · 2022-11-20T19:19:30Z

src/data/v2/last_quote.rs

 // TODO: Not all fields are hooked up.
-#[derive(Clone, Debug, Deserialize, Eq, PartialEq)]
+#[derive(Clone, Debug, Serialize, Deserialize, Eq, PartialEq)]


Why is this change necessary?

d-e-s-o · 2022-11-20T19:19:50Z

src/data/v2/last_quote.rs

 #[non_exhaustive]
 pub struct Quote {
  /// The time stamp of this quote.
-  #[serde(rename = "t")]


Why are these renames being removed?

d-e-s-o · 2022-11-20T19:20:43Z

src/data/v2/last_quote.rs

+/// A representation of the JSON data in the response
+#[derive(Debug, Deserialize)]
+pub struct LastQuoteResponse {
+  quotes: HashMap<String, QuoteDataPoint>,


Why a HashMap repesentation? We should likely expose what we get.

d-e-s-o · 2022-11-20T19:21:02Z

src/data/v2/last_quote.rs

-  fn path(input: &Self::Input) -> Str {
-    format!("/v2/stocks/{}/quotes/latest", input.symbol).into()
+  fn path(_input: &Self::Input) -> Str {
+    format!("/v2/stocks/quotes/latest").into()


Don't need to format if you are not formatting.

d-e-s-o · 2022-11-20T19:21:25Z

src/data/v2/last_quote.rs

  }

  fn parse_err(body: &[u8]) -> Result<Self::ApiError, Vec<u8>> {
    from_json::<Self::ApiError>(body).map_err(|_| body.to_vec())
  }
 }

-


Keep needless useless changes to a minimum?

d-e-s-o · 2022-11-20T19:21:46Z

src/data/v2/last_quote.rs

-
-    let quote = from_json::<Quote>(response).unwrap();
+			"quotes": {
+				"TSLA": {


Please no tabs in code using spaces?

sbeam · 2022-11-23T19:02:59Z

When you mentioned that back-compat wasn't a concern, I took that as more license to refactor than it seems you intended. I understand your points and appreciate the concern for quality and following the prior patterns.

Most of these changes were to make it work with the multiple quotes JSON, which has a different structure than the single quote response. So the way the response is deserialized and coerced into a Vec<Quote> had to be much different, and that was the best way I found to do it. However, we don't have to return a Vec<Quote>, it could be a HashMap<String, Quote>, which would be exactly what the JSON looks like and therefore simpler to implement.

That said, I'm most interested in getting the Last Trade part of this in. Would it be more agreeable to go back to the original plan, and copy the existing pattern into last_trade, using the single object API instead of multi? There is no reason we have to implement multi here -- it was kind of a "bonus" in my mind -- but it could be done as part of an followup later on.

d-e-s-o · 2022-11-24T00:25:09Z

That said, I'm most interested in getting the Last Trade part of this in. Would it be more agreeable to go back to the original plan, and copy the existing pattern into last_trade, using the single object API instead of multi? There is no reason we have to implement multi here -- it was kind of a "bonus" in my mind -- but it could be done as part of an followup later on.

In a way I coerced you into doing the last quote multi conversion, so perhaps we can do the following: I can take a closer look at getting last_quote converted into last_quotes in the next few days. Then you can build the last trade data retrieval in a similar vein. That should eliminate some of the guess work on your end and a bunch of communication overhead on mine and you can focus on the part you have an interest in. Does that sound reasonable?

sbeam · 2022-11-24T01:26:56Z

Yes, whichever way you want to go is fine of course. If you don't have time to convert this to multi then I'd be happy to put up another PR that just implements last_trade (single) and doesn't touch anything else.

d-e-s-o · 2022-11-24T04:06:32Z

Let me take a look tomorrow and then we'll take it from there.

d-e-s-o · 2022-11-25T06:58:00Z

Let me take a look tomorrow and then we'll take it from there.

Okay, it's mostly ready at https://github.com/d-e-s-o/apca/tree/topic/last-quote-multi. Will probably land that tomorrow and don't expect many mor echanges.

Edit: The changes are now in devel and I've deleted the aforementioned branch.

d-e-s-o · 2023-06-18T03:25:22Z

Closing this pull request as it hasn't seen any response or update.

d-e-s-o reviewed Nov 8, 2022

View reviewed changes

sbeam force-pushed the latest-trade-data branch from 208cdb2 to 6934b5a Compare November 15, 2022 19:34

d-e-s-o requested changes Nov 20, 2022

View reviewed changes

d-e-s-o closed this Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Latest trade data #43

Latest trade data #43

sbeam commented Oct 24, 2022

d-e-s-o commented Oct 25, 2022

sbeam commented Oct 25, 2022

d-e-s-o commented Oct 26, 2022

sbeam commented Oct 31, 2022

sbeam commented Nov 3, 2022

d-e-s-o left a comment

d-e-s-o Nov 8, 2022

d-e-s-o Nov 8, 2022

d-e-s-o Nov 8, 2022

sbeam Nov 15, 2022

sbeam commented Nov 15, 2022

d-e-s-o commented Nov 17, 2022

d-e-s-o left a comment •

edited

Loading

d-e-s-o Nov 20, 2022

d-e-s-o Nov 20, 2022

d-e-s-o Nov 20, 2022

d-e-s-o Nov 20, 2022

d-e-s-o Nov 20, 2022

d-e-s-o Nov 20, 2022

d-e-s-o Nov 20, 2022

sbeam commented Nov 23, 2022

d-e-s-o commented Nov 24, 2022

sbeam commented Nov 24, 2022

d-e-s-o commented Nov 24, 2022

d-e-s-o commented Nov 25, 2022 •

edited

Loading

d-e-s-o commented Jun 18, 2023

Latest trade data #43

Latest trade data #43

Conversation

sbeam commented Oct 24, 2022

d-e-s-o commented Oct 25, 2022

sbeam commented Oct 25, 2022

d-e-s-o commented Oct 26, 2022

sbeam commented Oct 31, 2022

sbeam commented Nov 3, 2022

d-e-s-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbeam commented Nov 15, 2022

d-e-s-o commented Nov 17, 2022

d-e-s-o left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbeam commented Nov 23, 2022

d-e-s-o commented Nov 24, 2022

sbeam commented Nov 24, 2022

d-e-s-o commented Nov 24, 2022

d-e-s-o commented Nov 25, 2022 • edited Loading

d-e-s-o commented Jun 18, 2023

d-e-s-o left a comment •

edited

Loading

d-e-s-o commented Nov 25, 2022 •

edited

Loading