cli icon indicating copy to clipboard operation
cli copied to clipboard

add Print* functions family to App

Open alessio opened this issue 3 years ago • 9 comments

Also redirect errors to stderr - as per POSIX recommendations.

What type of PR is this?

  • bug
  • feature

What this PR does / why we need it:

  • Add convenience Print{,Err}{,f,ln} functions to App.
  • Errors and diagnostics should always go the defined ErrorWriter (default: stderr), not stdout.

alessio avatar Nov 14 '20 16:11 alessio

Thanks for the contribution!

I'm not sure I agree with the idea that the app should have the Print* family of functions attached to it. We expose a writer and an error writer, so achieving the same functionality for the end-user is as simple as:

- a.PrintErrln(err)
+ fmt.Fprintln(a.ErrWriter, err)

We'd adding surface area to the API, which ups the maintenance burden and learning curve, but we're not adding functionality and not really saving folks more than a few keystrokes. I tend to be extremely conservative in terms of growing the interface, as some of the more innocuous looking changes we've added in the past have caused serious challenges as this library has evolved.

I do think the point about only printing errors to the ErrWriter is valid, and I think that would be a good subset of this PR to bring in.

rliebz avatar Nov 14 '20 16:11 rliebz

Hi @rliebz, and thanks for the quick feedback!

achieving the same functionality for the end-user is as simple as:

I don't disagree to tthat, my objective here was to propose adding readable convenience functions so that clients could be spared from extra boilerplate. IMvHO a.PrintErrln(err) is more conveniente and readable than fmt.Fprintln(a.ErrWriter, err). But I do understand that this could be a matter of taste :+1:

I do think the point about only printing errors to the ErrWriter is valid

Sweet, I'll open an alternative PR then. Thanks!

alessio avatar Nov 14 '20 16:11 alessio

@rliebz here you go: https://github.com/urfave/cli/pull/1204

alessio avatar Nov 14 '20 17:11 alessio

@alessio Can you pull in the default branch to fix the failing test please 🙏🏼

coilysiren avatar Jan 27 '21 18:01 coilysiren

Done @lynncyrin - thanks

alessio avatar Jan 27 '21 19:01 alessio

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

stale[bot] avatar Jun 02 '21 15:06 stale[bot]

Closing this as it has become stale.

stale[bot] avatar Jul 04 '21 04:07 stale[bot]

@alessio Hello and sorry for the long delay 😭 Has this PR been superseded by #1204 ?

meatballhat avatar Apr 21 '22 22:04 meatballhat

Not necessarily.

On Fri, 22 Apr 2022 at 00:10, Dan Buch @.***> wrote:

@alessio https://github.com/alessio Hello and sorry for the long delay 😭 Has this PR been superseded by #1204 https://github.com/urfave/cli/pull/1204 ?

— Reply to this email directly, view it on GitHub https://github.com/urfave/cli/pull/1203#issuecomment-1105810407, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABX73GQ7DBNU3HXFCWQJ7DVGHG6ZANCNFSM4TVT3BQA . You are receiving this because you were mentioned.Message ID: @.***>

-- Alessio Treglia | www.alessiotreglia.com Debian Developer | @.*** Ubuntu Core Developer | @.*** 0416 0004 A827 6E40 BB98 90FB E8A4 8AE5 311D 765A

alessio avatar Apr 22 '22 00:04 alessio

@urfave/cli I agree with @rliebz comment about not adding more surface API to cli code. Yes these functions are useful but not a real time saver in any meaningful sense. I want to close this PR without a merge

dearchap avatar Dec 05 '22 02:12 dearchap