Skip to content

Conversation

@kernel-sanders
Copy link
Contributor

This pull adds a -y flag which enables YAML output as well as a -z flag that outputs dates in ISO 8601 format. I'm using this to populate data on repos for a personal project so if these features don't interest you, no worries!

@o2sh o2sh requested review from o2sh and spenserblack January 9, 2021 23:25
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Overall, LGTM! ❤️ both YAML and ISO 😃

Left a suggestion on the CLI, let me know what you think 🙂

Comment on lines 70 to 74
.arg(
Arg::with_name("yaml")
.short("y")
.long("yaml")
.help("Outputs Onefetch in YAML format.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this conflicts with the --json flag, I think that either

  • conflicts_with("json") should be added (and also a conflicts_with("yaml") for the json Arg), or
  • The --json and --yaml flags should be replaced with a --format Arg with the possible values json and yaml

Personally, I think the latter would be better to keep the CLI as simple as possible 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

I'm working on the latter, I'll be pushing a commit soon.

@o2sh
Copy link
Owner

o2sh commented Jan 10, 2021

Thanks for your PR @kernel-sanders.
With a871c07, I took the liberty - as suggested by @spenserblack - to merge the --json flag and --yaml into a single CLI flag called output.

Everything else LGTM 👍

Copy link
Contributor Author

@kernel-sanders kernel-sanders 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! Moving the output logic into printers.rs cleans up main a bit as well.

Co-authored-by: Spenser Black <spenserblack01@gmail.com>
@o2sh o2sh merged commit 3e9cd24 into o2sh:master Jan 10, 2021
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.

3 participants