Skip to content

add Print* functions family to App#1203

Closed
alessio wants to merge 4 commits intourfave:mainfrom
alessio:print-functions
Closed

add Print* functions family to App#1203
alessio wants to merge 4 commits intourfave:mainfrom
alessio:print-functions

Conversation

@alessio
Copy link

@alessio alessio commented Nov 14, 2020

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 alessio requested a review from a team as a code owner November 14, 2020 16:05
@alessio alessio requested review from asahasrabuddhe and rliebz and removed request for a team November 14, 2020 16:05
As per POSIX recommendations.
@rliebz
Copy link
Member

rliebz commented Nov 14, 2020

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.

@alessio
Copy link
Author

alessio commented Nov 14, 2020

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 👍

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

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

@alessio
Copy link
Author

alessio commented Nov 14, 2020

@rliebz here you go: #1204

@coilysiren
Copy link
Member

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

@alessio
Copy link
Author

alessio commented Jan 27, 2021

Done @lynncyrin - thanks

@stale
Copy link

stale bot commented Jun 2, 2021

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 stale bot added the status/stale stale due to the age of it's last update label Jun 2, 2021
@stale
Copy link

stale bot commented Jul 4, 2021

Closing this as it has become stale.

@stale stale bot closed this Jul 4, 2021
@meatballhat meatballhat reopened this Apr 19, 2022
@meatballhat meatballhat added kind/bug describes or fixes a bug kind/feature describes a code enhancement / feature request area/v2 relates to / is being considered for v2 and removed status/stale stale due to the age of it's last update labels Apr 21, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 21, 2022
@meatballhat meatballhat changed the base branch from master to main April 21, 2022 22:09
@meatballhat
Copy link
Member

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

@alessio
Copy link
Author

alessio commented Apr 22, 2022 via email

@meatballhat meatballhat added status/waiting-for-response Waiting for response from original requester and removed status/user-feedback-required labels Oct 2, 2022
@dearchap
Copy link
Contributor

dearchap commented Dec 5, 2022

@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 dearchap closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug kind/feature describes a code enhancement / feature request status/waiting-for-response Waiting for response from original requester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants