Add `--json` export flag for issues and pull requests by mislav · Pull Request #3414 · cli/cli

mislav · 2021-04-13T18:35:33Z

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 for post-processing JSON just like in gh api.

References #385
Fixes #2306, fixes #1532

Limitations:

Not all GraphQL fields on issues/PRs are available. Only a preset of known fields are available for fetching. Use the gh api command to directly access parts of the GitHub API that are not exposed through this functionality.
When nested data is requested (e.g. labels on an issue, reviews on a pull request), we only request up to 100 records per collection since this is the maximum that the GitHub API will serve us.

TODO:

tests
enable --json for status command
add shared help section explaining --jq and --template flags

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`.

samcoe

Overall I really like this functionality and it worked great while testing it out. Most of my comments revolve around how we could change some of the structure of the code to be easier to test and have a greater separation of responsibilities.

samcoe · 2021-04-13T20:51:29Z

api/queries_issue.go

 	Body           string
 	CreatedAt      time.Time
 	UpdatedAt      time.Time
+	ClosedAt       *time.Time


Should this be time.Time? Feels odd that the other fields are not pointers but this one is.

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.

samcoe · 2021-04-13T21:09:23Z

api/query_builder.go

+	"statusCheckRollup",
+)
+
+func PullRequestGraphQL(fields []string) string {


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.

samcoe · 2021-04-13T21:47:19Z

pkg/cmdutil/json_flags.go

+	Template string
+}
+
+func (e *ExportFormat) Write(w io.Writer, data interface{}, colorEnabled bool) error {


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.

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.

Probably could move this all into the export package as well.

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.

samcoe · 2021-04-13T21:52:34Z

pkg/cmd/issue/list/list.go

 	Browser    browser

 	WebMode bool
+	Export  *cmdutil.ExportFormat


I think this would be a bit clearer named ExportFormat or ExportFmt.

samcoe · 2021-04-13T21:53:12Z

pkg/cmd/issue/status/status.go

 	IO         *iostreams.IOStreams
 	BaseRepo   func() (ghrepo.Interface, error)
+
+	Export *cmdutil.ExportFormat


Same ExportFormat naming comment as above.

samcoe · 2021-04-13T22:17:18Z

pkg/cmdutil/json_flags.go

+	error
+}
+
+func AddJSONFlags(cmd *cobra.Command, exportTarget **ExportFormat, fields []string) {


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.

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.

samcoe · 2021-04-13T22:36:59Z

api/queries_pr.go

+	ClosedAt         *time.Time
+	MergedAt         *time.Time


Same question about pointers as above.

…utput

samcoe

Thanks for making the requested changes! This looks good to me. I just added some comments about adding some non-happy-path case tests.

samcoe · 2021-04-14T17:11:26Z

pkg/cmd/root/help.go

+	if longText == "" {
+		longText = command.Short
+	}
+	if longText != "" && command.LocalFlags().Lookup("jq") != nil {


Seems like this might have meant to look up the json flag instead of the jq flag?

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.

samcoe · 2021-04-14T17:14:17Z

api/export_pr_test.go

+		outputJSON string
+	}{
+		{
+			name:   "simple",


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.

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.

samcoe · 2021-04-14T17:18:30Z

api/query_builder_test.go

+		want   string
+	}{
+		{
+			name:   "empty",


I think we could add one additional test for when the fields requested are invalid.

mislav added 7 commits April 13, 2021 16:48

Extract JSON filtering functionality from gh api

61a8049

Move issue list queries to under the issue/list package

19ea49b

Merge remote-tracking branch 'origin' into json-format

298ef8a

Restructure PullRequestStatus function

e158fac

Add pr status --json support

3f22e3b

Add issue status --json support

a516ee6

vilmibm requested review from samcoe and vilmibm April 13, 2021 19:53

voanhcung approved these changes Apr 13, 2021

View reviewed changes

samcoe reviewed Apr 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

milkly1235writer approved these changes Apr 13, 2021

View reviewed changes

mislav added 5 commits April 14, 2021 17:28

Expose more fields for PR JSON export

e63904b

Change JSON Exporter to an interface

3ad41e3

Add gh help formatting topic & link to it from commands with JSON o…

e327b42

…utput

Tests for GraphQL query builder and JSON exporter

78e48e2

Merge remote-tracking branch 'origin' into json-format

7ec5b0f

mislav requested a review from samcoe April 14, 2021 16:54

Fix assigning null Exporter

625505d

samcoe approved these changes Apr 14, 2021

View reviewed changes

mislav added 3 commits April 14, 2021 19:41

Fix pr view tests broken by createdAt → submittedAt switch

56cdbb6

Disallow unsupported values for --json flag

654bd29

Fix whitespace formatting of issue/pr view help text

516ea86

mislav merged commit 31cccb4 into trunk Apr 14, 2021

mislav deleted the json-format branch April 14, 2021 18:15

mislav mentioned this pull request Apr 23, 2021

gh pr view --json not returning date fields #3497

Closed

This was referenced May 11, 2021

Push data serialization concern into Exporter #3621

Merged

Add --json export functionality to repo commands #3627

Merged

Add release view --json export support #3656

Merged

This was referenced May 12, 2021

Add --json export functionality to repo commands #3627

Merged

Add release view --json export support #3656

Merged

landfillbaby mentioned this pull request Oct 15, 2021

use builtin jq HaywardMorihara/gh-tidy#19

Merged

andyfeller mentioned this pull request Nov 13, 2024

The pr view command with the --json files flag omits some of the files changed #9916

Open

Conversation

mislav commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samcoe Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment has been minimized.

samcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mislav commented Apr 13, 2021 •

edited

Loading

samcoe Apr 13, 2021 •

edited

Loading