cli
cli copied to clipboard
add Print* functions family to App
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.
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.
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!
@rliebz here you go: https://github.com/urfave/cli/pull/1204
@alessio Can you pull in the default branch to fix the failing test please 🙏🏼
Done @lynncyrin - thanks
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.
Closing this as it has become stale.
@alessio Hello and sorry for the long delay 😭 Has this PR been superseded by #1204 ?
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
@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