architv / soccer-cli

:soccer: Football scores for hackers. :computer: A command line interface for all the football scores.
MIT License
1.09k stars 222 forks source link

Replace reading auth token from a file with using an environment variable #33

Closed ueg1990 closed 8 years ago

ueg1990 commented 8 years ago

Refer to Issue 31

I think this is a much better way of reading an auth token.

@architv For ensuring it reads during PIP installation, you can refer to this link: https://pip.pypa.io/en/latest/user_guide.html#environment-variables

Saturn commented 8 years ago

Setting it this way can make things a bit more complicated. What would be the recommended way for a Windows user to set the variable? So that it persists.

If a user is installing it 'system-wide' and not inside a virtualenv, then could the variable name API_TOKEN clash with something else?

I have seen some programs supporting both an API_TOKEN from a config file and also one from an environment variable. It first checks to see if an environment variable exists and if not, it will try and read it from the config file. Best of both worlds?

I actually quiite like the untracked authtoken.py since I can move between branches and virtualenvs without touching it. Obviously I could just set it in my .profile or something but it feels more self contained this way.

Also what is the deal if someone installs from PyPI? Because doesn't football-data.org work without an API_TOKEN. It just has a low rate limit.

ueg1990 commented 8 years ago

For those who install from PyPI they will still have to add an env variable. But thats like the same with authtoken.py.

As for Windows, here is a way to set env variables: https://docs.python.org/2/using/windows.html#excursus-setting-environment-variables

I think a separate ticket should be opened to discuss replacing the *.py used for loading teams and leagues...i really do not think they should be this way (and i personally do not like it). maybe we can look at some other examples on github for references?

Saturn commented 8 years ago

I think a separate ticket should be opened to discuss replacing the *.py used for loading teams and leagues...i really do not think they should be this way (and i personally do not like it). maybe we can look at some other examples on github for references?

I agree.

architv commented 8 years ago

@ueg1990 Wouldn't adding an environment variable make it difficult for a novice to setup the app. Currently if someone installs the app from PyPi, they don't need to do anything more. And also as @Saturn mentioned If a user is installing it 'system-wide' and not inside a virtualenv, then could the variable name API_TOKEN clash with something else?

As far as having python dictionaries for loading leagues and teams is concerned, even I agree that it's not a very good approach. I am going to merge leagueids.py and leagueproperties.py any way. Let's look for some references for this.

ueg1990 commented 8 years ago

So currently, when someone downloads from PyPi, they use your token account? If they do, that is not a good thing i think because if a lot of users (holefully :smile: ) start using soccer-cli, then the rate limit can be exceeded very quickly. and your users will definitely be developers (even novice); so they should be able to do it. and we will definitely help them.

For the variable name thing, we should change the name of the key; maybe we can make it SOCCER_CLI_API_TOKEN.

architv commented 8 years ago

@ueg1990 Yes, that seems right. I think we should follow a config file/environment variable method to load the API key. @Saturn pointed out, we could do something like this:

try:
    import config
    api_key = config.footballdata['api_key']
except:
    api_key = os.environ.get('SOCCER_CLI_API_KEY')

if not api_key:
    import sys
    print 'No config.py file found. Exiting.'
    sys.exit(0)

What do you think?

Saturn commented 8 years ago

I think supporting both ways would work.

Maybe if no API_KEY is found it could print the part about visiting the correct site and registering for a key. To make it even easier for a user who is lost.

ueg1990 commented 8 years ago

@architv ill add the changes today. Can you just explain one thing: import config api_key = config.footballdata['api_key']

is it reading from the file config.py? or footballdata.py? I do not know what is config reading from?

architv commented 8 years ago

@ueg1990 It's reading from the file config.py. footballdata is a dictionary with a key api_key.

ueg1990 commented 8 years ago

ok cool. thank you :)