Closed lbstr closed 3 years ago
For the sake of discussion, here's the rough approach I'm taking:
import { Client } from '@master-chief/alpaca';
import { parseAccount } from './parsers';
// your package
const alpaca = new Client({ ... });
const accountRaw = await alpaca.getAccount();
// new code that I'm talking about...
const account = parseAccount(accountRaw);
const buyingPower = account.buying_power_num; // number!!!
Questions:
Im all for translating the fields to their native values we would typically work with. Date strings to dates, prices and numbers to ... numbers.
Im away at the moment but ill be back next week and will take a look at how this can be approached simply. Im personally in favor of the package doing the translation by default.
Sounds good, thank you! Feel free to reach out if you want to bounce ideas around on different approaches.
If you're thinking about adding fields like buying_power_num
alongside the existing buying_power
for example, that should be pretty quick and I can help if you'd like. If you're thinking about just making buying_power
a number
instead of a string
, that's obviously a breaking change. I checked a couple of the other unofficial SDKs (Java and C++) and they leave numbers as strings, but the Java one parses dates, so there isn't a clear precedent for this.
I have other breaking changes in mind that I could put up PRs for if you're interested in working towards a 2.0.0 release. For example, I would love to see union types for certain strings. Looks like this is already done in some places, which is great.
Let's be opinionated on this one. The package should be native to the language first (not the Alpaca API) and feel right at home out of the box. If someone wanted a 1:1 mirror of the Alpaca API with raw responses they can make the requests themself easily. Id love to see the PR's, a 2.0 release with breaking changes to certain field types is ok with me. It gets rid of redundant work in the long run.
Merged those changes into dev
.
Just ran into this issue as well. It seems rather odd that the value return from the Alpaca API for the buying_power
property of the account
is a string. I'm glad we are handling this in this package. Great job!
@KalebMills @lbstr I have been testing the github:117/alpaca#dev
version on paper for the last two days, haven't run into any bugs.
Amazing timing. I just put up the next PR! There's still more to do, but making progress...
I noticed on the #dev
branch that the Parser
is constructed in the Client
constructor. What are thoughts on making the parser optional, or injectable? I.e, what if I wanted to make additional changes to the data returned from the API, or totally different from what the new Parser
does. Could we instead create an interface that could be used to create custom Parser's
off of? Alternatively, what if people don't want the data changed?
@KalebMills I agree that that flexibility sounds nice. Especially the ability to turn off parsing entirely and just surface the raw data from Alpaca.
If we go that route, the challenge is in the return types of the Client
. Some reasonable options:
RawClient
and ParsingClient
. Kind of gross.Client<A, O, P>
where A
is the Account type, O
is for Orders, P
is for Positions and so on. Just quickly scanning through the code, I think there would be 10ish. The same generic types would exist on the Parser
so you could provide your own parser that conforms to the types. Probably best to split Parser
into IAccountParser<T>
and so on where T
is the type of the parsed Account. Clunky, but super flexible.Parser
as a helpful tool, but the actual usage of it would be in user code. Dead simple for users to get what they want, but we lose the nice auto-parsing that most users probably want.Do you have any opinions on these or any other ideas?
@lbstr
parse
function? That way, each has the ability to be parsed, and it does not impact the current functionality. This also give the user the ability to "enable/disable" parsing with each method. @KalebMills Just want to make sure I'm following your comment for 3...
Something like this?
const client = new Client();
const rawAccount = await client.getAccount();
const account = rawAccount.parse();
console.log(typeof rawAccount.buying_power); // string
console.log(typeof account.buying_power); // number
This reminds me... Option 4 is something that I brought up early on in this issue. Just have a single Account
type that has both the parsed and unparsed values side-by-side. For example, Account
would have buying_power
and buying_power_num
. Or maybe the inverse: buying_power
and buying_power_str
. Nothing to configure. You get the unparsed value if you want to use it. The downside is that there are extra fields -- if you are storing tons of data, this might be something you care about.
Yep exactly like that!
Agreed with your point on your idea, that seems like a lot of additional data to cache. Ideally, instead the caller could simply parse the returned data if they so desired, while also having access to the initial data types. Double the fields does seem a bit more complicated to convey to users, instead of being able to have a single place in the README to explain what the .parse()
function does on certain methods.
With this approach, you have all the usual data fields, along with the ability to transform the the fields to their better types.
What's wrong with parsing it as the intended native value? A number string gets parsed to a number and a date string parsed to a Date and so on. If someone wants the raw string I'd argue that's the least used format for users of a package like this, I've never not parsed a number value that I needed to work with as a number, nor do i leave date strings as dates when I need to work with them. What is the use case that we are building around? Functionality for programatic trading or... something else? Im just not understanding the opposing viewpoint of leaving them as strings at all. 🤔
@117 I'm mostly playing devil's advocate here because I will 100% want numbers over strings for my needs, but...
The Alpaca docs touch on why they use strings...
Decimal numbers are returned as strings to preserve full precision across platforms. When making a request, it is recommended that you also convert your numbers to strings to avoid truncation and precision errors.
While I agree that most people using this will just want numbers, it feels wrong to completely circumvent Alpaca's intent here. If a TypeScript user wants strings so that they have guaranteed precision across platforms, it would be a shame that they couldn't use this package.
@117 I'm mostly playing devil's advocate here because I will 100% want numbers over strings for my needs, but...
The Alpaca docs touch on why they use strings...
Decimal numbers are returned as strings to preserve full precision across platforms. When making a request, it is recommended that you also convert your numbers to strings to avoid truncation and precision errors.
While I agree that most people using this will just want numbers, it feels wrong to completely circumvent Alpaca's intent here. If a TypeScript user wants strings so that they have guaranteed precision across platforms, it would be a shame that they couldn't use this package.
It makes sense.
What if we used something like this https://github.com/MikeMcl/decimal.js/ ...? I already use it internally when precision math is needed.
@117 Agreed that 99% of users would probably prefer the native types. @lbstr Perhaps, we do the inverse of my suggestion. Maybe by default provide the parsed types, then provide a function to return the native types?
@KalebMills OK I just put up a PR that explores that idea. Let me know what you think.
OK I think we're good with this issue. We've parsed all of the numbers and added union types for special strings. We will handle dates in #15 . Thanks for the feedback!
Question Given that the Alpaca API provides dates and numbers as strings, where do you feel the responsibility lies for parsing those values?
Background Right now this package provides types that match the Alpaca API 1:1. For example, in
Account
, we havebuying_power
, which is astring
. From the developer's perspective, this is nice because there are no surprises. I can look at the Alpaca Docs and trust that this package will provide me data in the same fashion.If I want to use
buying_power
, I need to safely convert it to anumber
. From a typing perspective, we would ideally have two types: one that matches the Alpaca API and one that has numbers and dates parsed. This package provides the former, but I'm curious about the latter.Right now I'm basically wrapping your client and doing the mapping myself. For example, I have an
AccountParsed
type which extendsAccount
and hasbuying_power_num: number
.Since everyone has to do this, I feel that it should be shared. I would love your feedback this because I'm not sure if it belongs here or in a separate package.