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

cmd: fix outputting resources for CVS mode if only one exists. #166

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

fatih
Copy link
Member

@fatih fatih commented Apr 19, 2021

This PR fixes the --format csv output for commands that only output a single item. The gocsv package is not able to print individual structs and expects a slice of structs. We fix the issue by explicitly wrapping individual items with a slice.

There is a side effect, though. This changes how we print in JSON mode: instead of printing a single item, it's now printing a list with a single item in it.

Before

$ pscale database show planetscale --format csv
Error: cannot use database.Database, only slice or array supported

After:

$ pscale database show planetscale --format csv
Name,CreatedAt,UpdatedAt,Notes
planetscale,1614012928571,1617039724414,You're looking at it.

fixes: https://github.com/planetscale/project-big-bang/issues/224

This PR fixes the `--format csv` output for commands that only output a
single item. The `gocsv` package is not able to print individual structs
and expects a slice of structs. We fix the issue by expliclity wrapping
individual items with a slice.

There is a side effect though. This changes how we print in JSON mode:
instead of printing a single item it's now printing a list with a single
item in it.

fixes: planetscale/surfaces#224
@fatih fatih requested a review from a team as a code owner April 19, 2021 10:40
@iheanyi
Copy link
Member

iheanyi commented Apr 19, 2021

Is there another way of doing this? Could we perhaps do this hack only at the CSV level?

@fatih
Copy link
Member Author

fatih commented Apr 19, 2021

Is there another way of doing this? Could we perhaps do this hack only at the CSV level?

I checked the gocsv package and it doesn't support any options to change it. We could use an if/else clause for each show function, but that introduces more code than anticipated:

Instead of:

return ch.Printer.PrintResource(toDatabases([]*ps.Database{database}))

We only wrap if it's CSV:

if ch.Printer.Format() == printer.CSV {
	return ch.Printer.PrintResource(toDatabases([]*ps.Database{database}))
} else {
	return ch.Printer.PrintResource(toDatabase(database))
}

@fatih
Copy link
Member Author

fatih commented Apr 19, 2021

There is another solution, but this solution requires quite a work. Before I choose gocsv, we were using encoding/csv from the stdlib. One problem with the package is, it's not able to Marshall a struct or a slice of structs. Instead one has to use a slice of [][]string records. This would mean that each struct had to implement a method or function that would return a slice of records. But adding it to every single resource wasn't easy and required quite a work. We started using gocsv to get rid of it.

But after writing it, I think we could use a mixture of both approaches. Let me check locally whether it's worth it.

@fatih
Copy link
Member Author

fatih commented Apr 19, 2021

@iheanyi check this out: a7251a0

Copy link
Member

@iheanyi iheanyi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for doing this!

@fatih fatih merged commit 8c1950c into main Apr 19, 2021
@fatih fatih deleted the fatih/fix-csv-output branch April 19, 2021 14:59
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.

2 participants