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

refactor: Create Printer object to manage output #1292

Closed
wants to merge 3 commits into from

Conversation

TerryHowe
Copy link
Member

What this PR does / why we need it:

Create a printer object to manage output

Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 93.10345% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 82.02%. Comparing base (f1d319f) to head (4417131).
Report is 19 commits behind head on main.

❗ Current head 4417131 differs from pull request most recent head 1db763f. Consider uploading reports for the commit 1db763f to get more accurate results

Files Patch % Lines
cmd/oras/root/login.go 66.66% 1 Missing and 2 partials ⚠️
cmd/oras/root/discover.go 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1292      +/-   ##
==========================================
+ Coverage   81.94%   82.02%   +0.07%     
==========================================
  Files          83       83              
  Lines        4005     4022      +17     
==========================================
+ Hits         3282     3299      +17     
  Misses        500      500              
  Partials      223      223              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TerryHowe TerryHowe changed the title Printer refactor refactor: Create Printer object to manage output Mar 17, 2024
@@ -184,7 +185,7 @@ func pushManifest(cmd *cobra.Command, opts pushOptions) error {
}
return opts.Output(os.Stdout, descJSON)
}
status.Print("Pushed", opts.AnnotatedReference())
printer.Print("Pushed", opts.AnnotatedReference())
Copy link
Contributor

@qweeah qweeah Mar 19, 2024

Choose a reason for hiding this comment

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

@TerryHowe Is this blocking your implementation of ORAS Golang scripting? IMHO this output should be classified to metadata output and it's better to user metadata push handler rather than new printer:

_, displayMetadata := display.NewPushHandler("", nil, opts.Verbose) // avoiding formatting and TTY output
displayMetadata.OnCopied(&opts.Target)

Copy link
Member Author

Choose a reason for hiding this comment

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

What I'm trying to develop here is a layer similar to logrus that doesn't know what is is printing, it just knows how to print. To me at least, ideally the format code would be a layer on top of that.

Copy link
Contributor

@qweeah qweeah Mar 20, 2024

Choose a reason for hiding this comment

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

Gotcha. Any good idea to inject the writer in? The writer might come from STDOUT or cobra command output.

@qweeah
Copy link
Contributor

qweeah commented Mar 26, 2024

@TerryHowe I added a new Printer interface to display package to unify the output into STDOUT.

// Printer prints.
type Printer interface {
Printf(format string, a ...any) (n int, err error)
Println(a ...any) (n int, err error)
PrintJSON(object any) error
ParseAndWrite(object any, templateStr string) error
Outputable
}

It provides PrintXXX to wrap fmt.FprintXXX utilities, also provides PrintJSON and ParseAndWrite for formatted output

To reflect your changes in #1288, cobra.Command.OutOrStdout can be injected via below interface

// Outputable interface is used to reset output of the handler.
type Outputable interface {
// WithOutput resets output of the handler.
WithOutput(out io.Writer)
}

Please help you help review related changes in #1310 and let me know your comment on this design, thanks.

@qweeah
Copy link
Contributor

qweeah commented Apr 1, 2024

@TerryHowe I have added another solution in #1314, please let me know if it meets your expectation.

@TerryHowe TerryHowe marked this pull request as draft April 8, 2024 23:58
@TerryHowe TerryHowe closed this May 6, 2024
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