Fix regression caused by resolveOptions

[chelnak]

If opts was nil, resolveOptions would only update the local copy of opts. The pointer was never being updated and was therefore nil when passed to the New*Client methods.

Closes #17

[samcoe]

Thanks for catching this and thanks for the fix. I would like to see this solved in a slightly different manner though. To me it doesn't make sense to be passing in nil to resolveOptions since it works by modifying the passed in options. Rather we should solve this by fixing the callers to make sure they are passing in valid api.ClientOptions. Does that make sense? Additionally, if you are feeling up for it it would be great to add a test or two for resolveOptions. I should have added some when I created the function but overlooked it.

[chelnak]

Hey, that sounds good. I'll push some changes + new tests this afternoon.

Cheers,
Craig

[chelnak]

@samcoe I was wondering if it was still desirable to allow the consumer to pass nil for ClientOptions.

e.g:
gh.HTTPClient(nil)

Then it would be up to the client methods to pass a valid api. ClientOptions.

func HTTPClient(opts *api.ClientOptions) (*http.Client, error) {
	if opts == nil {
		opts = &api.ClientOptions{}
	}
.....

The other option would be to ask he consumer to provide an empty api.ClientOptions.

[samcoe]

My thought would be that it should be up to the client methods to pass a valid api.ClientOptions to resolveOptions even if they received nil, so exactly as you proposed 👍

[chelnak]

I've pushed an implementation. I couldn't decide whether to include a nil check in resolveOptions so I left it out. I don't think it will ever be an issue to the consumer given that the client methods now handle the opts. Saying that though.. I'm more than happy to add something in if it's preferable!

Currently pondering on tests... it feels tricky because of config.Load() and mocking it would mean exporting some more things from the config package..

or we could pass in config to resolveOptions e.g

func resolveOptions(clientOptions **api.ClientOptions, cfg config.Config) error {
...

however I'm fairly fresh with Go so I might be overlooking something obvious.

[chelnak]

Added some tests and rebased 👍

[samcoe]

Great work @chelnak, thanks for adding tests. Code looks good. I pushed one small change removing the double pointer argument that was remnants from the first approach and was unnecessary now.

[chelnak]

@samcoe appreciated 😄