sofarocean / sofar-api-client-python

Python Client for Wavefleet
Apache License 2.0
7 stars 5 forks source link

bug/custom_api_token #8

Closed mattp0 closed 3 years ago

mattp0 commented 4 years ago

The __init__.py does not handle the custom_token flag properly. The configuration of env tokens is hardcoded into the operations of __init__.py. This can be fixed with a simple movement of the config settings into the get_token function. Similarly, the endpoint has the same issue. Although no constructor param has been created yet but moving the endpoint configuration into the get_endpoints() will allow for future features.

I was attempting to use this lib to showcase via a jupyter notebook to show off the capabilities. The current __init__.py is hardcoded to always read the auth token from os.path.expanduser("~") In the context of a notebook this is sort of annoying and requires a hacky fix to move files from within the notebook.

I thought, it might be nice to contribute to this project but adding a flag for custom tokens outside the normal env load. To my delight, I found it has already been started. It appears that there is a bug though, the flag is ignored as it is always hard coded. Starting at line 14 of __init__.py I noticed that the config values are not within a function and are always called. It appears to me that they should have been placed in the get_token() function. `# config values userpath = os.path.expanduser("~")

enviromentFile = os.path.join(userpath, 'sofar_api.env')

dotenv.load_dotenv(enviromentFile)

token = os.getenv('WF_API_TOKEN')`

Based on this code def __init__(self, custom_token=None): self._token = custom_token or get_token() on line 38 of 'init.py`

I am purposing that the code for generating the config should be placed into the get_token(). This should allow for API objects to be generated with a custom_token.

mattp0 commented 4 years ago

I have made the changes to fix this bug, stuck on testing to ensure that nothing seems to be broken, as testing is linked to a certain spotter and API Token. PR is here

mike-sosa-sofarocean commented 3 years ago

Hey there. Sorry for the delay. We've been a bit busy with a few different projects but I'll try to get the fix for this merged in ASAP