airbrake / airbrake-python

Airbrake Python
MIT License
51 stars 24 forks source link

Improve init signature #57

Open zachgoldstein opened 7 years ago

zachgoldstein commented 7 years ago

There are a number of options to improve on the current init signature. Right now it's a list of parameters with defaults.

What's wrong with the current situation?

The only real alternative I can think of is to pass the context by ref in one arg and create it separately from the Airbrake object.

stavxyz commented 7 years ago

The only real alternative I can think of is to pass the context by ref in one arg and create it separately from the Airbrake object.

This sounds like a fine idea to me.

zachgoldstein commented 7 years ago

The only thing holding me back here is looking at the other approaches from rollbar and sentry. Looks like they both use **kwargs, https://github.com/rollbar/pyrollbar/blob/master/rollbar/__init__.py#L263 and https://github.com/getsentry/raven-python/blob/master/raven/base.py.

I'm starting to think we should stick with **kwargs, but in the readme suggest that users create a separate dict object with all the config, then pass that in. I think it's nice to be able to init the library in one line, and forcing config to another structure might be a tad inconvenient.

stavxyz commented 7 years ago

This might be a question of what you expect from your SDK. If I pass in a context key/value pair that airbrake doesn't support, should I get an error when instantiating the notifier class? Or will it bomb out (possibly in a background thread after https://github.com/airbrake/airbrake-python/issues/50) when the airbrake api returns a non-2xx ?

If the SDK code should keep track of and check the context schema at initialization time, then there ought to be updates to the code here if the airbrake api changes or supports new (or when it deprecates) the values supported in context.

FWIW, my preference is when the SDK is stupid-proof when possible, even if that means a little bit of overhead, both for the maintainers of the library or the clients using airbrake.

I am not super opinionated, I just want to help out if I can. I still think it is wise to avoid a grab-bag variable.

In python3, a user could certainly build their own dictionaries, and pass them in using multiple times, like `Airbrake(..., context, **config)` which is nice.

We have to change the signature every time we add new options

It's fair to want to avoid this for the Airbrake class, but perhaps there could be a utility/helper function that will help you build a context dictionary with some useful defaults.

zachgoldstein commented 7 years ago

If I pass in a context key/value pair that airbrake doesn't support, should I get an error when instantiating the notifier class?

Ya, I think errors at init time are ideal here, we should add a check for this.

If the SDK code should keep track of and check the context schema at initialization time, then there ought to be updates to the code here if the airbrake api changes or supports new (or when it deprecates) the values supported in context.

Yup, agreed.

It's fair to want to avoid this for the Airbrake class, but perhaps there could be a utility/helper function that will help you build a context dictionary with some useful defaults.

I think that's a great idea. So ideally this function would also: