honeybadger-io / honeybadger-go

Send Go (golang) panics and errors to Honeybadger.
https://www.honeybadger.io/
MIT License
34 stars 15 forks source link

Move several types to pointers #2

Closed mperham closed 8 years ago

mperham commented 9 years ago

There's a couple of spots where you are mixing pointers and value types for core types like Configuration and Context. For instance, your Configuration type is a pointer here but a value here, forcing you to awkwardly deference it. Unless you are explicitly trying for a functional-ish API (i.e. immutable), I'd suggest moving to use pointers exclusively for composite types; it ensures you aren't spending extra cycles copying data needlessly and can mutate method parameters.

joshuap commented 9 years ago

Ah yeah, the reason I did that is the configuration is passed as a pointer to several other instances such as the worker here. If I change the pointer on the client instance it would not update any of the associated pointers (so I opted to change the reference instead, which does make the configuration mutable). Since the configuration can be updated at runtime I was having a hard time deciding how to rebuild other types which depend on it -- I think to be immutable it would need to create a new worker, which is a plausible alternative.

kostyantyn commented 8 years ago

@joshuap what do you think about using Configuration as a pointer everywhere.

For instance, creating a copy of a client, you could pass the configuration from existing one. client := honeybadger.New(oldClient.Config)

right now you must dereference the pointer: client := honeybadger.New(*oldClient.Config)

What is the reason that honeybadger.New accepts Configuration as a value and not as a pointer?

joshuap commented 8 years ago

I think I can do that; it will just require rebuilding the instances which depend on the configuration when Configure is called, otherwise the issue is that the Configuration values change but the objects (like the worker) which already exist don't get the updated values. I'll play with this a bit.

joshuap commented 8 years ago

I fixed the issue of dereferencing the config in order to update it. I think we're basically not going the functional route, so this seems simpler.

@kostyantyn regarding passing a value rather than a pointer to things like honeybadger.Configure() and honeybadger.New(), I did that because the argument in those cases is used to update the pointer config which is created on the returned Client instance. Passing a pointer would not reduce the number of copies, and I think honeybadger.Configure(honeybadger.Configuration{APIKey: "your api key"}) is a simpler API than honeybadger.Configure(&honeybadger.Configuration{APIKey: "your api key"}) (having to reference a pointer). Thoughts?