Parsing strings about alpaca-ts HOT 19 CLOSED

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.

from alpaca-ts.

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!

from alpaca-ts.

@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.

from alpaca-ts.

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.

from alpaca-ts.

@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?

from alpaca-ts.

@KalebMills OK I just put up a PR that explores that idea. Let me know what you think.

from alpaca-ts.

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:

• Do you agree that this is something everyone has to do?
• Where do you feel like it should go? Separate package? In here?
• Is there something I'm missing?

from alpaca-ts.

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.

from alpaca-ts.

Merged those changes into dev.

from alpaca-ts.

Amazing timing. I just put up the next PR! There's still more to do, but making progress...

from alpaca-ts.

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?

from alpaca-ts.

@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:

1. Have multiple clients like RawClient and ParsingClient. Kind of gross.
2. Generic type params for each return type like 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.
3. Don't parse at all in the client -- always return raw data. Provide the 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?

from alpaca-ts.

@lbstr

1. Agree with you on this
2. This seems the most logical, like you said as well also clunky. This does seem to accomplish the task, and the default types could simply be the parsed data types.
3. I think the auto-parsing is desirable most of the time. Combined with what I said for #2, here's another thought: Could the parseable functions return the data along with a 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.

from alpaca-ts.

@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.

from alpaca-ts.

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.

from alpaca-ts.

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. 🤔

from alpaca-ts.

@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.

from alpaca-ts.

@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.

from alpaca-ts.

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!

from alpaca-ts.