Add --json export flag for issues and pull requests
#3414
Conversation
The `--json` flag accepts a list of GraphQL fields to query for and output in JSON format. To get the list of available flags, run the command with a blank value for `--json`. Additional `--jq` and `--template` flags are available just like in `gh api`.
| @@ -33,6 +30,7 @@ type Issue struct { | |||
| Body string | |||
| CreatedAt time.Time | |||
| UpdatedAt time.Time | |||
| ClosedAt *time.Time | |||
There was a problem hiding this comment.
Should this be time.Time? Feels odd that the other fields are not pointers but this one is.
There was a problem hiding this comment.
The other time fields are guaranteed to be present (non-null), but this one isn't, and by marking it a pointer we grant it a possible null state. This matter for the exported JSON, because otherwise a zero-value timestamp would get exported.
| "statusCheckRollup", | ||
| ) | ||
|
|
||
| func PullRequestGraphQL(fields []string) string { |
There was a problem hiding this comment.
I think we can come up with a better name for this function, maybe FieldsToGraphQL or something. It is used for Issue so using PullRequest in the name was a bit confusing.
pkg/cmdutil/json_flags.go
Outdated
| Template string | ||
| } | ||
|
|
||
| func (e *ExportFormat) Write(w io.Writer, data interface{}, colorEnabled bool) error { |
There was a problem hiding this comment.
I think we could make this easier to use and more testable with an interface. I was thinking something like
type Exporter interface {
Export(io.Writer, ExportFormat, interface{}) error
}Then we can do something similar to Browser where the default one gets set in the factory, or specified in the NewCmd___ functions, and overridden in tests where needed.
Additionally I think the colorEnabled option should be part of the ExportFormat.
There was a problem hiding this comment.
Another possibility is that the Exporter interface could do the work that api/export_pr.go is doing now. The interface could have additional functions like ExportIssues and ExportPullRequests that match up with the ExportData functions there and then those functions could call the Export function once the issue/pr has been massaged into the right format.
There was a problem hiding this comment.
Probably could move this all into the export package as well.
There was a problem hiding this comment.
Good point about the interface! I have changed this to an interface per your recommendation.
All of your suggestions are pretty good, but in this short time period I won't have time to refactor how the exporter works (e.g. to fold export_pr.go into the exporter functionality). This would be a good follow-up item.
pkg/cmd/issue/list/list.go
Outdated
| @@ -31,6 +31,7 @@ type ListOptions struct { | |||
| Browser browser | |||
|
|
|||
| WebMode bool | |||
| Export *cmdutil.ExportFormat | |||
There was a problem hiding this comment.
I think this would be a bit clearer named ExportFormat or ExportFmt.
pkg/cmd/issue/status/status.go
Outdated
| @@ -19,6 +19,8 @@ type StatusOptions struct { | |||
| Config func() (config.Config, error) | |||
| IO *iostreams.IOStreams | |||
| BaseRepo func() (ghrepo.Interface, error) | |||
|
|
|||
| Export *cmdutil.ExportFormat | |||
There was a problem hiding this comment.
Same ExportFormat naming comment as above.
pkg/cmdutil/json_flags.go
Outdated
| error | ||
| } | ||
|
|
||
| func AddJSONFlags(cmd *cobra.Command, exportTarget **ExportFormat, fields []string) { |
There was a problem hiding this comment.
It looks like we often want to use the json field passed in by the user or a set of default fields, what do you think about passing the default fields into this function and using them as the default for the json flag? If we did this we would probably also want to make sure that in the case when the json flag is not specified we don't return nil but rather a ExportFormat struct with some indication, maybe a new field, that states if a non-default format was specified.
There was a problem hiding this comment.
It's a good thought but I'd like to avoid specifying the default at the flag level because the flag serves two purposes at once: to opt into JSON export mode, and to control what data is fetched. If there was a default at the flag level, the reader might mis-interpret it that those default fields somehow pertain to the export, whereas they don't.
I'm fine for now passing default fields through a separate mechanism, because normal fetch represents a different mode of operation than the --json mode.
| ClosedAt *time.Time | ||
| MergedAt *time.Time |
There was a problem hiding this comment.
Same question about pointers as above.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
| if longText == "" { | ||
| longText = command.Short | ||
| } | ||
| if longText != "" && command.LocalFlags().Lookup("jq") != nil { |
There was a problem hiding this comment.
Seems like this might have meant to look up the json flag instead of the jq flag?
There was a problem hiding this comment.
Good catch! But I also wanted to apply this help text addition to the gh api command, which has --jq and --template but no --json.
| outputJSON string | ||
| }{ | ||
| { | ||
| name: "simple", |
There was a problem hiding this comment.
I think it would be nice to add two additional tests. One where a field is requested that is not in the input JSON just to verify that it does not blow up and a second where the field requested is not a valid field on an issue. Same goes for the PullRequest tests.
There was a problem hiding this comment.
Good call! I've added an assertion that it's impossible to specify invalid fields in --json arguments. That should take care of unsanitized values making it through to the exporter.
As for the GraphQL builder and exporters themselves, they support arbitrary values so that they are easily extensible with new fields without special-casing them. However, requesting an invalid field would likely fail on the GraphQL request phase.
| want string | ||
| }{ | ||
| { | ||
| name: "empty", |
There was a problem hiding this comment.
I think we could add one additional test for when the fields requested are invalid.
The
--jsonflag accepts a list of GraphQL fields to query for and output in JSON format. To get the list of available flags, run the command with a blank value for--json. Additional--jqand--templateflags are available for post-processing JSON just like ingh api.References #385
Fixes #2306, fixes #1532
Limitations:
gh apicommand to directly access parts of the GitHub API that are not exposed through this functionality.TODO:
--jsonfor status command--jqand--templateflags