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

inspect: remove format flag #155

Merged
merged 3 commits into from
Nov 28, 2022

Conversation

hectorj2f
Copy link
Contributor

Signed-off-by: Hector Fernandez [email protected]

Summary

We didn't set any default format, so it is hard to know which options are available for the format.

Release Note

Fix: set default format value for the inspect' format flag.

Documentation

Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f hectorj2f requested a review from a team as a code owner November 25, 2022 11:55
@hectorj2f
Copy link
Contributor Author

There is something else missing... pushing another commit

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2022

Codecov Report

Merging #155 (cc4cfce) into main (f91d0d1) will decrease coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   50.11%   50.00%   -0.12%     
==========================================
  Files          18       18              
  Lines         846      844       -2     
==========================================
- Hits          424      422       -2     
  Misses        376      376              
  Partials       46       46              
Impacted Files Coverage Δ
cmd/timestamp-cli/app/inspect.go 25.92% <ø> (-5.11%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hectorj2f
Copy link
Contributor Author

timestamp-cli inspect --timestamp_server http://localhost:5555 --timestamp response.tsr I don't think we need to set the --format as a required flag, especially when there is a single option.

@hectorj2f
Copy link
Contributor Author

ping @cpanato

@haydentherapper
Copy link
Contributor

If we don’t have any other format supported, id just say to delete the flag.

@hectorj2f
Copy link
Contributor Author

I wasn't sure, there were other plans for the format flag.

@hectorj2f
Copy link
Contributor Author

If there aren't, then let's remove it.

@haydentherapper
Copy link
Contributor

YAGNI, we can add it back later if we add more formatting options!

Signed-off-by: Hector Fernandez <[email protected]>
@hectorj2f
Copy link
Contributor Author

@haydentherapper I removed the flag.

@haydentherapper haydentherapper changed the title inspect: set default format to default inspect: remove format flag Nov 28, 2022
@haydentherapper haydentherapper merged commit 06993cb into sigstore:main Nov 28, 2022
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