Expand logging support for HTTP traffic #60
Conversation
- Respect the GH_DEBUG environment variable by default, but add the option to skip it; - Add option to opt into colorized logging; - Add option to explicitly log HTTP headers and bodies—the new default is off; - Colorize JSON payloads when logging HTTP bodies; - Log form-encoded payloads; - Pretty-print GraphQL queries when logging HTTP requests.
| // LogIgnoreEnv disables respecting the GH_DEBUG environment variable for logging. | ||
| LogIgnoreEnv bool | ||
|
|
||
| // LogColorize enables colorized logging to Log for display in a terminal. |
There was a problem hiding this comment.
| // LogColorize enables colorized logging to Log for display in a terminal. | |
| // LogColorize enables colorized logging to Log for display in a terminal. | |
| // Default is no coloring. |
With the comments I have been trying to describe the default state for each options as I generally find it useful when I look at documentation.
| @@ -0,0 +1,143 @@ | |||
| package jsonpretty | |||
There was a problem hiding this comment.
Do we want this package to be exported? If so, can we add a comment to the top here describing the purpose and functionality of the package?
| @@ -38,6 +38,15 @@ type ClientOptions struct { | |||
| // Default is no logging. | |||
| Log io.Writer | |||
|
|
|||
| // LogIgnoreEnv disables respecting the GH_DEBUG environment variable for logging. | |||
| LogIgnoreEnv bool | |||
There was a problem hiding this comment.
After thinking about this more, I am not sure that this option is necessary. I understand its purpose, but can't think of a good use case where we want to allow an extension author to ignore GH_DEBUG.
Additionally, I think our users would find the behavior of this option a bit confusing without further comments describing its behavior.
There was a problem hiding this comment.
After thinking about this more, I am not sure that this option is necessary. I understand its purpose, but can't think of a good use case where we want to allow an extension author to ignore
GH_DEBUG.
Thanks for chiming in. One consumer of go-gh that wants to use this option is the CLI itself, since GitHub CLI already parses environment variables itself and wants to prevent underlying libraries to parse the same environment variables, lest some inconsistencies or conflicts arise.
Additionally, an extension author might want to disable env-controlled logging if it already offers the user granular controls for logging.
As a general rule of thumb, if a generic Go library automatically respects environment variables, it's a good idea to offer a way for the consumer of said library to skip respecting environment variables in cases where that is not desired. At the end of the day, environment variables are essentially global variables shared across modules and even processes, and to mitigate their potential harms, developers should have ways to put up boundaries to limit the propagation (or effect) of these variables.
I have added some extra documentation detailing this configuration option.
There was a problem hiding this comment.
That all makes sense to me, my only thought was that if an extension author wanted to ignore GH_DEBUG they could provide a value for the Log option making LogIgnoreEnv feel a bit redundant.
Co-authored-by: Sam Coe <samcoe@users.noreply.github.com>
The central idea of this change is that GH_DEBUG environment variable is respected the same way it is in gh CLI.