Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

feat: add typescript support via typings file #83

Merged
merged 4 commits into from
Sep 19, 2018

Conversation

evanshortiss
Copy link
Contributor

This will add support for typescript users and (I believe) should also add improved intellisense for JS users in VSCode.

I added the .editorconfig file since my editor was defaulting to 4 spaces whereas the project seems to use 2. The .editorconfig will help enforce that.

@evanshortiss
Copy link
Contributor Author

Resolves #81

@coveralls
Copy link

coveralls commented Sep 17, 2018

Coverage Status

Coverage remained the same at 44.118% when pulling 46a736e on evanshortiss:ts-types into 71457ba on aurbano:master.

robinhood.d.ts Outdated
* @param code
* @param callback
*/
set_mfa_code (code: string, callback: InitCallback): void
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is referenced in the docs but is not in the code I see in master. Should it be here or removed?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should either remove it from here or add it back in the code if there is still support for it I suppose

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I can remove for now and can add back if it changes.

robinhood.d.ts Outdated
Day = 'day'
}

enum TriggerTypes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to open an issue for this, but I think TimeInForceTypes and TriggerTypes are reversed according to https://github.com/sanko/Robinhood/blob/master/Order.md and the tests that I was doing the other day. Side note - I wish 'ioc' time_in_force worked but maybe Robinhood deprecated it because I was getting invalid value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chiefsmurph you're correct, not sure how I managed that! I can address this later PDT.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this is ready to merge and I'll release a new version with it

@evanshortiss
Copy link
Contributor Author

@aurbano this is ready now. The MFA references are removed until the authentication stuff is resolved - I'll gladly PR it back when required.

The README might also need an update since it still references it.

@aurbano
Copy link
Owner

aurbano commented Sep 19, 2018

@evanshortiss awesome I'll merge, edit the README and release a new version with it 😄

@aurbano aurbano merged commit 344dbfb into aurbano:master Sep 19, 2018
@evanshortiss
Copy link
Contributor Author

Thanks @aurbano

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants