-
Notifications
You must be signed in to change notification settings - Fork 52
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
cmd: introduce printer, helper and new --format flag #151
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of changes in here, but most of them are very similar. I've read through and everything looks good to me, but with this much code, I can't certain that everything is perfect. I think that's fine however, it's better to ship this now and get feedback.
The one thing it'd be great to find a solution for would be the end of tons of commands looking like this:
if ch.Printer.Format() == printer.Human {
ch.Printer.Printf("Schema snapshot %s was successfully created!\n", printer.BoldBlue(snapshot.Name))
return nil
}
I don't have a better option, but it feels like a bit of a leaky abstraction. I don't think it should hold the PR though!
if ch.Printer.Format() == printer.Human { | ||
ch.Printer.Printf("Backup %s was successfully created!\n", printer.BoldBlue(bkp.Name)) | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have a pattern for something like:
if ch.Printer.Format() == printer.Human { | |
ch.Printer.Printf("Backup %s was successfully created!\n", printer.BoldBlue(bkp.Name)) | |
return nil | |
} | |
ch.Printer.HumanPrintf("Backup %s was successfully created!\n", printer.BoldBlue(bkp.Name)) |
That would allow to not have conditionals, and that method could just do nothing if the human printer wasn't configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, print only works if Human is enabled 🤦🏼♂️ So the guards are not needed at all!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, now that I think, this will output two things, the message and also the table (because that's enabled for human as well). So in this case, this is how it would look like:
$ pscale branch create planetscale fatih-can-be-deleted
Branch fatih-can-be-deleted was successfully created!
NAME STATUS PARENT BRANCH CREATED AT UPDATED AT NOTES
---------------------- -------- --------------- ------------ ------------ -------
fatih-can-be-deleted main now now
I thought a lot about this, but not sure what the best idea would be. Happy to improve it after merging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I don't really have a great answer. I would merge this now before I put too much thought into it honestly.
This PR changes how we print information and deserialize API resources from the CLI. With that, this PR also adds JSON and CSV formatting options for every single subcommand.
It's a significant change, but it completely changes how we print human-readable information to the user and how we encode resources in various formats, such as JSON or CSV. Here are the important changes:
Updated
internal/printer
packageThis package is completely rewritten. It provides the following new methods to print information:
Instead of the
fmt
package, we're going to use these commands going forward inside the subcommands to print information to the user. These commands work differently that we have control of thestdout/stderr
references. By default, they all print tostdout
, but we can easily switch them to/dev/null
or anywhere else (such as saving to a file).The benefit of controlling the output destination comes in handy because we can decide whether the
PrintXXX
methods are no-op or not. This is especially useful for enabling full JSON mode, where we don't want to output accidentally non-JSON data to stdout (see the issue as an example: https://github.com/planetscale/project-big-bang/issues/197).How we do dynamically switch of the stdout or disable it? Let me show it with the
printer.Print()
example (the same logic applies to the otherprinter.PrintXXX
methods):The important bit is the combination of
fmt.Fprint
, which allows us to choose the destination to output and theprinter.out() io.Writer
function that returns the destination. As you see, by default, we returnos.Stdout
, but that changes toioutil.Discard
, which means to discard the output (think of it like/dev/null
).Now let's move to the
printer.Format
, which configures the output format and also changes the switch inprinter.out()
Changing the output format via
printer.Format
In the core of the new
printer.Printer
type is the newly createdprinter.Format
type:The
printer.Format
type is an enum that currently has three values:Human
: this is the default value used when you executepscale
. Humans meanhuman-readable
output. This can be either a single line of information, such asSuccessfully created database 'foo'
or a table that lists resources (e.g., a list of databases).JSON
: outputs in JSON format.CSV
: outputs in CSV format.This
format
value is set with the--format
and the-f
flags and passes as a pointer to theprinter.Printer
struct:The value can be later retrieved with the
printer.Printer.Format()
method and make decisions based on the format. As an example, here is how we use it to print a human friendly message:Printing a resource
Printing a piece of information, such as
Fetching existing databases...
and printing a resource, is not the same. To print an API object, such as a database resource, we need to represent it in serializable data format. The types we receive from theplanetscale-go
are already serializable, but sometimes we want to control and omit specific fields or annotate them with additional information. Hence each subcommand is responsible for its serialization format. This means that the printer types are now moved into their appropriate subcommand package and no longer a part of theprinter
package.To print a resource, such as a
branch
orlist of deploy requests
, we use the followingprinter.PrintResource()
function:The center of the function is again the
printer.format
field, which decides how we want to serialize and print the given resource. We still use the existing table format for typehuman
, but if the user changes the format via--format json
or--format csv
, we use the appropriate encoders.As a side note, we use a custom CSV encoder that can print a struct or a slice of structs. The stdlib
encoding/csv
requires more work and is not very intuitive to use.A new helper method for the subcommand
Previously we would pass the
*config.Config
to each subcommand. This has changed now. To help subcommands facilitate their actions in a deterministic way, we're now passing a helper struct that contains the printer as well:It's a struct instead of a new argument because we don't want to break existing subcommands when extending with new fields.
Putting everything together
Assuming we have a command that prints the branch status with the following print methods:
Now, here there are two print methods, one being
printer.Printf()
and the other one isprinter.PrintResource
. By default, the--format
flag is set toprinter.Human
, which means the output will be in the form:However, when the user sets the format to
--format json
, then theprinter.PrintXXX()
methods will print toioutil.Discard
(which means theGetting status ...
message will be discarded) andprinter.PrinterResource
will print the deserialized resource to stdout:By making sure that all
printer.PrintXXX()
methods discard their output, we can safely output in JSON format without mangling it with other messages.closes https://github.com/planetscale/project-big-bang/issues/197
closes https://github.com/planetscale/project-big-bang/issues/168
closes https://github.com/planetscale/project-big-bang/issues/179
closes https://github.com/planetscale/project-big-bang/issues/202
closes https://github.com/planetscale/project-big-bang/issues/201