Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async for issue #11 #51

Merged
merged 25 commits into from
Mar 14, 2024
Merged

Async for issue #11 #51

merged 25 commits into from
Mar 14, 2024

Conversation

4rumprom
Copy link
Contributor

@4rumprom 4rumprom commented Mar 5, 2024

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

Copy link
Owner

@itsjafer itsjafer left a comment

Choose a reason for hiding this comment

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

The code itself looks good to me. I'm happy to merge this in, see if the tests work as expected, and then push to PyPi.

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

schwab_api/authentication.py Outdated Show resolved Hide resolved
@4rumprom
Copy link
Contributor Author

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.

Correcting typo and adding robustness
@4rumprom
Copy link
Contributor Author

4rumprom commented Mar 12, 2024

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.

changed .*app/trade.* to re.compile(r"app/trade")
@4rumprom
Copy link
Contributor Author

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

@itsjafer
Copy link
Owner

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

@4rumprom
Copy link
Contributor Author

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

@itsjafer itsjafer merged commit 263be42 into itsjafer:main Mar 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants