materialsproject / api

New API client for the Materials Project
https://materialsproject.github.io/api/
Other
107 stars 39 forks source link

Implementation of MP_API_KEY environment variable results in odd usage pattern #741

Closed bsjoelin closed 1 year ago

bsjoelin commented 1 year ago

The type annotations for MPRester states that api_key: str | None = DEFAULT_API_KEY. This is incorrect, since giving api_key = None fails with MPRestError stating "message":"No API key found in request".
This is due to api key then actually being None instead of the value of the MP_API_KEY environment variable, as one would expect.

So, when using MPRester inside functions, where you want to give the user the ability to give their API key as an optional argument you must give mp_api.client.mprester.DEFAULT_API_KEY as the default value. Thus, your code will end up looking something like this:

from typing import Optional
from mp_api.client import MPRester
from mp_api.client.mprester import DEFAULT_API_KEY

def function(arg1, arg2, ..., api_key: Optional[str] = DEFAULT_API_KEY):
    ...
    with MPRester(api_key) as mpr:
        ...

Instead of the more sensible choice that the type annotations suggest:

from typing import Optional
from mp_api.client import MPRester

def function(arg1, arg2, ..., api_key: Optional[str] = None):
    ...
    with MPRester(api_key) as mpr:
        ...
munrojm commented 1 year ago

@bsjoelin this is a sensible point. Thanks for the feedback. I will make the appropriate change.

munrojm commented 1 year ago

This should be fixed.