Allow skipping option and header resolution for api clients #56
Conversation
| if tz != "" { | ||
| headers[timeZone] = tz |
There was a problem hiding this comment.
This alone looks like it's sufficient to solve the original issue (sending the blank Time-Zone header). How come additional changes to the functionality are needed?
There was a problem hiding this comment.
The reason that is not sufficient to solve the bug is because this line is explicitly telling go-gh not to resolve the "Time-Zone" header and leave it set to an empty string. We could detect that in go-gh and remove the "Time-Zone" header if it set to an empty string but that felt off since we have been following the rule that any user supplied values are not modified. Adding the SkipResolution options feels like a cleaner solution and also addresses the confusion you correctly identified here.
There was a problem hiding this comment.
Fair enough, but if there is going to be an additional config option, maybe it could be better documented? Right now I don't think it's clear what "resolution" is, since the user probably isn't familiar with the implementation of go-gh. Since the configuration option only affects whether default headers will be added, maybe the option could be called skipDefaultHeaders?
There was a problem hiding this comment.
Great suggestion, that is a much more descriptive name.
| if tz != "" { | ||
| headers[timeZone] = tz |
There was a problem hiding this comment.
Fair enough, but if there is going to be an additional config option, maybe it could be better documented? Right now I don't think it's clear what "resolution" is, since the user probably isn't familiar with the implementation of go-gh. Since the configuration option only affects whether default headers will be added, maybe the option could be called skipDefaultHeaders?
This PR adds the
SkipResolutionoption toapi.ClientOptionsallowing the user to skip all automatic resolutions thatgo-ghwill do upon initializing an api client. This PR also changes the "Time-Zone" header to only be added if it was properly resolved, both changes are necessary for fixing 5978.Fixes cli/cli#5978
cc cli/cli#5935