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.

Sign up for GitHub

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

feature(websocket): Add websocket client #508

Merged
merged 14 commits into from
Jul 19, 2021
Merged

feature(websocket): Add websocket client #508

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4ce17b8
Added barebone support for websockets API
Mohamedemad4 Sep 2, 2020
1910c4e
added docs to README
Mohamedemad4 Sep 2, 2020
febcd8a
added a more informative docstring with response examples;more verbos…
Mohamedemad4 Sep 6, 2020
111dcb0
ammends to the last commit
Mohamedemad4 Sep 6, 2020
a76fa92
minor comments and doc changes
Mohamedemad4 Sep 10, 2020
e5b89b3
Added tests to check for valid initialization arguments for TiingoWeb…
Mohamedemad4 Sep 10, 2020
fa6b010
Added tests to check for valid initialization arguments for TiingoWeb…
Mohamedemad4 Sep 10, 2020
d9cb894
ditched the Globals and used a higher order func instead;also ditched…
Mohamedemad4 Oct 3, 2020
716a5b6
removed uneeded import
Mohamedemad4 Oct 4, 2020
f157b0c
removed config as a default arg;added comment to explain why on_error…
Mohamedemad4 Oct 6, 2020
be25aa2
better error handling for config option
Mohamedemad4 Jan 3, 2021
a15986f
test of missing API key in os env and config dict
Mohamedemad4 Jan 12, 2021
0be8171
ignore lgtm [py/not-named-self]
Mohamedemad4 Jul 1, 2021
b162391
use unittest.mock to remove TIINGO_API_KEY
Mohamedemad4 Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion tiingo/wsclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def cb_fn(msg):
while True:pass
'''

def __init__(self,config={},endpoint=None,on_msg_cb=None):
def __init__(self,config,endpoint=None,on_msg_cb=None):
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for updating this line, to complete the change we should also add the following ternary

self.config = {} if config is None else config

Otherwise, we'll get an error when we try to run self.config.update as update isn't available on a None type.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why the travis checks aren't showing in the Github UI, but you can check the test results from the Travis ui here:

https://travis-ci.org/github/hydrosquall/tiingo-python/jobs/733171252

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but like. that should only happen if someone called TiingowebsocketClient without the config param. python would raise an error. because the param doesn't have a default argument. so it won't even reach the self.config.update.
am i getting this right?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, you're right - I have been working mostly in JS recently, and forgot that in Python, if you don't pass in an argument that you'll get an error at runtime (in JS, if you don't pass in an argument, the function runs but assumes that the value of config === undefined. I supposed the value of doing the check would have been to guard against someone doing TiingoWebsocketClient(config=None).

Anyways, I think that having a named argument is helpful to callers so they know the meaning of the position, so I guess the full change would look like:

def __init__(config=None, # other args)

    self.config = {} if config is None else config

Regarding the failing test, it appears that the error wasn't raised because the test environment contained the TIINGO_API_KEY environment variable. In order to complete the change, you can follow the delenv example here:

monkeypatch.delenv

https://docs.pytest.org/en/4.6.1/monkeypatch.html#monkeypatching-environment-variables

In case you haven't run the unit tests locally before, that can be done by running the following in your terminal when in the root directory, this will be faster than waiting for the tests to run in CI.

python setup.py test

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @Mohamedemad4, just wanted to check if you might have time to take a look at fixing this test - I think it's the last blocking item that we'd need before we can include your changes in the new release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the link you provided is broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also i don't get why the CI tests are failing? shouldn't the CI be providing it's own TINGO_API_KEY as an env var? or am i missing something?

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @Mohamededmad4, thanks for looking into this. The CI environment does have a TIINGO_API_KEY. The test is failing because the TIINGO_API_KEY is provided, which is why test_missing_api_key doesn't work.

tiingo-python/.github/workflows/python-package.yml

Lines 42 to 43 in f6ef473

env:
TIINGO_API_KEY: 0000000000000000000000000000000000000000

As a result, the code uses that environment variable and we never get to test the "RuntimeError" check for when the config is empty AND nothing is specified in the environment variables. To fix it, you can either change the logic of the client code, or delete the environment variable just inside of that test.

tiingo-python/tiingo/wsclient.py

Lines 46 to 56 in be25aa2

try:
api_key = self.config['authorization']
except KeyError:
api_key = os.environ.get('TIINGO_API_KEY')
self.config.update({"authorization":api_key})
self._api_key = api_key
if not(api_key):
raise RuntimeError("Tiingo API Key not provided. Please provide"
" via environment variable or config argument."
"Notice that this config dict takes the API Key as authorization ")

Let me know if that's still confusing. The updated link for how to patch environment variables is here:

https://docs.pytest.org/en/stable/monkeypatch.html#monkeypatching-environment-variables


self._base_url = "wss://api.tiingo.com"
self.config=config
Copy link
Owner

Choose a reason for hiding this comment

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

We're getting a warning from LGTM about mutating a basic object. I think we can solve it by doing

https://lgtm.com/rules/4840097/

  • Make the initial value None in the init constructor statement
self.config = {} if config is None else config

This should address the warning.

Expand Down Expand Up @@ -88,6 +88,7 @@ def on_msg_cb_local(ws,msg):
return
return on_msg_cb_local

# since methods need to be unbound in order for websocketClient these methods don't have a self as their first parameter
def on_error(ws, error):
Copy link
Owner

Choose a reason for hiding this comment

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

Let's make the first argument to on_error and on_close self rather than ws, to address the warning provided by LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's just going to be confusing. since these functions don't get self when they are called. and will just receive a ws instance. i think a better approach would be to just use *args so as not to confuse future maintainers.BUT i doubt that would stop the LGTM linter from throwing a warning

Copy link
Owner

Choose a reason for hiding this comment

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

I see your point about not wanting people to think that "self" refers to the Tiingo client instead of the ws instance - what do you think about:

  • Leaving the function as is, but leaving a comment to explain to future maintainers why the variable is called ws on this line. This won't affect LGTM but will help maintainers
  • Removing this function from the TiingoClient class and defining it as a standalone function above the class - this will solve the LGTM issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's go with the first solution. i think it's best for this case

Copy link
Owner

Choose a reason for hiding this comment

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

If you see this in time, can we also add a comment to suppress the warning?

https://lgtm.com/help/lgtm/alert-suppression#python

If not no worries, I'll add this before releasing to PyPi on Friday - this is to avoid future contributors from getting spammed by a warning they can ignore.

print(error)

Expand Down