Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add json to list cmd #379

Merged
merged 6 commits into from
Sep 25, 2023
Merged

Conversation

catdevman
Copy link
Contributor

@catdevman catdevman commented Sep 21, 2023

This should close #380

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just dropped some requests for minor changes on the PR.

We should also add a test. We've been using txtar/testscript for it. However, we can add the test case out of band.

}

return errors.Wrap(table.Render(), "failed to render")
if !isJSON {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably break these out in functions (data passed to both is the same) for json and table respectively since the listCmd keeps growing.

},
}

cmd.PersistentFlags().BoolVar(&isJSON, "json", false, "This flag tells the list command to print the output in json")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename isJSON to formatJSON and move it into the function scope, just like here:

https://github.com/stateful/runme/blob/main/internal/cmd/fmt.go#L12-L13

Flags that are being used across commands can go into the module scope.

if err != nil {
return err
}
err = jsonpretty.Format(io.Out, bytes.NewReader(by), " ", false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine jsonpretty has some benefits over just writing to output? Either way, the dependency to go-gh already exists so no harm done.

@catdevman
Copy link
Contributor Author

Changes:

  • changed isJSON to formatJSON
  • moved row struct top level of the file so it get be used throughout
  • moved displaying table and json into functions that take iostreams.IOStreams and []row
  • added txtar/testscript test for runme ls --json

Copy link
Member

@sourishkrout sourishkrout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@sourishkrout
Copy link
Member

Tidied the modules and ran gofumpt (make fmt) which C/I will enforce. Merging on green build @catdevman.

@sourishkrout sourishkrout merged commit bc507f2 into stateful:main Sep 25, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow json output for the list command
2 participants