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

feature: Add a App.Reader that defaults to os.Stdin #1191

Merged
merged 1 commit into from Oct 22, 2020

Conversation

@stellirin
Copy link
Contributor

@stellirin stellirin commented Oct 2, 2020

What type of PR is this?

(REQUIRED)

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

(REQUIRED)

  1. Add a new default Reader to the App for easy testing STDIN.
  2. Adds tests for the above.

Which issue(s) this PR fixes:

(REQUIRED)

Closes: #1190

Release Notes

(REQUIRED)

Add a App.Reader that defaults to os.Stdin.
@stellirin stellirin requested a review from urfave/cli as a code owner Oct 2, 2020
@stellirin stellirin requested review from saschagrunert and rliebz and removed request for urfave/cli Oct 2, 2020
@rliebz
rliebz approved these changes Oct 3, 2020
Copy link
Member

@saschagrunert saschagrunert left a comment

LGTM

@TimothyStiles
Copy link

@TimothyStiles TimothyStiles commented Oct 22, 2020

I've been searching hours for a nice solution for testing stdin like this. Is there anyway I can help get it merged?

@rliebz rliebz merged commit 30bb698 into urfave:master Oct 22, 2020
12 checks passed
12 checks passed
ubuntu-latest @ Go 1.12
Details
ubuntu-latest @ Go 1.13
Details
ubuntu-latest @ Go 1.14
Details
macos-latest @ Go 1.12
Details
macos-latest @ Go 1.13
Details
macos-latest @ Go 1.14
Details
windows-latest @ Go 1.12
Details
windows-latest @ Go 1.13
Details
windows-latest @ Go 1.14
Details
test-docs
Details
codecov/patch Codecov Report
Details
codecov/project Codecov Report
Details
@TimothyStiles
Copy link

@TimothyStiles TimothyStiles commented Oct 22, 2020

Thanks @rliebz for merging that and thanks @stellirin for writing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants