itsjafer / schwab-api

A python library for placing trades on Charles Schwab
MIT License
201 stars 64 forks source link

Async for issue #11 #51

Closed 4rumprom closed 5 months ago

4rumprom commented 6 months ago

Hi itsjafer,

You mentioned code readability as the main issue and not wanting to support sync & async simultaneously. My initial thought was to to cancel the pull request but I then realized it was pretty straight forward to go fully async.

This will solve issue #11 and is backwards compatible.

I ran this code on streamlit which is incompatible with sync and also tested it with github actions to make sure async works. (cf. test.yml under workflows).

4rumprom commented 6 months ago

Does it make sense for this to a major version bump? i.e. from 0.3.X to 0.4.X because of the async support

I'd think so. Especially since the option strategies would be released with this version as well.

4rumprom commented 5 months ago

I am also experiencing login issues today. It seems that the url to place trades has changed, at least on my end. I have made a change to the login to ensure more robustness by looking for the keywords "app/trade" in the url instead of an exact url.

await self.page.wait_for_url(".*app/trade.*") # Making it more robust than specifying an exact url which may change.

EDIT: had to change: await self.page.wait_for_url(".*app/trade.*") # Making it more robust than specifying an exact url which may change. to await self.page.wait_for_url(re.compile(r"app/trade"), wait_until="domcontentloaded") # Making it more robust than specifying an exact url which may change.

4rumprom commented 5 months ago

Hi Jafer, Is there something missing on my end to complete this? Tks, Romain

itsjafer commented 5 months ago

Are you not able to merge this in? I thought once I approve the PR, it can be merged

4rumprom commented 5 months ago

Not 100% sure. This is all pretty new to me. It does say that only those with write access to this repository can merge pull requests.

image