Make create_session() api_key param optional

[bpepple]

Looks like the create_session() func use a non-optional api_key parameter. It would be easier to use downstream if you make that optional and if the information is not available throw an exception. For example, if your are using a settings file for you project it make sense to generate one on initial use, and then allow to provide the information at a later instance.

https://github.com/Buried-In-Code/Simyan/blob/76ed857b899712be2912bb6b64ed0add3898e09e/simyan/__init__.py#L14

it would be much nicer to use something like this:

def create_session(api_key: Optional[str] = None, cache: Optional[SQLiteCache] = None) -> Session:

[Buried-In-Code]

I'm not sure there's much use for having it Optional, as you cant use the API without one.
If you need to have it setup so that you create a session before loading/creating an API Key then you could always set it to blank or placeholder. But I would just change the order so that you dont create a session til you have loaded/created the details.

Having it as Optional makes it appear as though part or all of the API can be used without an API Key.

[bpepple]

Having it as Optional makes it appear as though part or all of the API can be used without an API Key.

That's exactly why it should be throwing an exception if it's not provided.

[Buried-In-Code]

If I've missed something let me know.

I don't think its worth changing it for 2 reasons:

1. By having it as Optional and throwing an exception if none is passed then it'll lead to the upstream getting the exception while running. But if I leave it as is then the error will be visible in the IDE before the end user experiences it.
2. By having it as Optional if new settings are loaded where the user hasnt set their API key yet and a none passed, the exception will be raised and the session object wont be created. But if I leave it as is then even if its not a valid API key the session object will still be created and so the API key can be adjusted at any point.

[bpepple]

What you are proposing is to let the end users deal with an error that is with this library. And really, I don't quite understand what you are referring to when you mention the upstream getting the exception.

Regarding your second point, why even create the session if the necessary information isn't present? And I don't understand how exactly are you going to change the api_key after the session object is created? What your are proposing is raising an ApiError in the request call if the key isn't present which seems like a really poor way of handling it, since you are unnecessarily calling the ComicVine api.

The whole point of raising an exception is to make it easier for users of the library with errors that might occur with the library. For example, to initialize simyan it would be much easier to do this for the users of the library than to have to write code to work around the design of simyan.

from simyan import create_session, exceptions
try:
    s = create_session(api_key=None)
except exceptions.AuthenticationError:
    print("No api_key set")
    exit(0)

[Buried-In-Code]

What you are proposing is to let the end users deal with an error that is with this library.

It wouldn't be the end user it'd just be the library users, I don't see this as an error with the library, but rather an enforcement that the user of the library has to follow, the enforcement being you need to have an API key before calling the library.

What your are proposing is raising an ApiError in the request call if the key isn't present which seems like a really poor way of handling it, since you are unnecessarily calling the ComicVine api.

Yea this isn't the best design choice of mine, but that's due to just raising an APIError regardless of the ComicVine error, but I'm already working on the solution to this in that it will raise an AuthenticationError if it gets an invalid API key response from ComicVine. As I can't As there's not much validation I can do on the string itself without knowing how ComicVine creates it's keys.

I would have thought the simpler solution would have been something like, rather than raising an exception:

settings = Settings()
if not settings.api_key:
    settings.api_key = input("Input API Key")
session = create_session(api_key=settings.api_key)

By having it as Non-Optional it discourages library users from writing code that allows the end user to pass a None as they'll get the warning:

The part that's confusing me the most is why let the user input a None only to then throw an exception instead of just stopping the user from inputting None in the first place.

[bpepple]

Well, feels like we are going in circles over something that should be fairly minor, and I really don't feel like expending much more effort on this, and really that's the beauty of open-source it's easy enough to fork something if you want to go another way than upstream.

[Buried-In-Code]

Fair enough, thanks for your input