alpacahq / alpaca-ts

A TypeScript Node.js library for the https://alpaca.markets REST API and WebSocket streams.
ISC License
154 stars 42 forks source link

Parse numbers in Account entity #10

Closed lbstr closed 3 years ago

lbstr commented 3 years ago

First step at Issue #9 -- just tackling the Account entity in this PR. The goal is to find an approach we like before moving on to the rest of the entities.

Overview

The general approach is to split the existing Account entity type into two types: RawAccount and Account.

Then we have a simple Parser class that is responsible for converting from RawAccount to Account. In the Client's getAccount() method, we simply make the API request, then call the parser. That's pretty much it.

Questions

Branch

I think you'll want to make a 2.x branch that we can merge this (and future PRs) to correct? Once you do that, I can update this PR to go there rather than master.

Prettier thing

The Prettier build step is failing. It looks like maybe it can't handle this PR because it's coming from my fork? Do you know if that's expected?

Tests

I wanted to write tests for the parser, but I couldn't get AVA up and running quickly without making a bunch of changes. I guess we'll add tests later?

Parsing Dates

I chose not to parse dates in this PR. I'm worried about timezones. As soon as you go from string to Date, you get a date that is in the timezone of whatever machine parsed it. This is a source of bugs. For example, you might have one machine hit the API and another machine process the data and those machines could be in different timezones.

We could parse dates and inform users of this package to be careful with timezones, but I believe it's safer to leave them as ISO-8601 strings and let the user do the parsing themselves.

Thoughts?

General style

Feel free to nitpick on stuff in here as I get used to the project's style. I do feel like this project would benefit from some lint rules, although maybe that's a non-issue since there is the Prettier action thing.

Manual testing

I npm linked my fork locally to my trading app. I hit the getAccount() method in the client and logged the output. The output matched my expectations.

Screen Shot 2020-09-11 at 3 56 27 PM
117 commented 3 years ago

Awesome! Can you point this to the new dev branch?

Building Not sure why the prettier build step is failing but I'll look into it, maybe its a one time thing. If not I'll replace it.

Tests As for tests... how we do this remains a mystery to me currently. The best thing I can think of is to mock alpaca responses in unit tests. Otherwise they aren't very useful if they aren't simulating API responses. I don't like the idea of requiring an API key and secret just to run the tests, so that is where mocking would come into play. That said it's a time consuming task and I don't think its necessary at the moment. This latest build has been very stable and I've used it thoroughly.

Dates What if dates were a string object that had a helper method, one that could convert it to a local date? Such as .toDate(). Another simple solution that comes to mind is to parse the dates ourself but use UTC which is more than reasonable. The user can take it to another timezone then on their own if they wish.

I'll await the PR on the new branch and then begin testing from there. Thank you by the way for your work on the project! Love to see the improvements taking shape. 😄

117 commented 3 years ago

Ok turn's out I can do it. Didn't know until I just checked. 🤷🏻‍♂️ Gonna go ahead and merge into dev and begin testing these changes.

lbstr commented 3 years ago

Thanks @117 ! I'm going to start working on the next one.