Skip to content
This repository has been archived by the owner on Dec 29, 2021. It is now read-only.

Add stdin function #44

Merged
merged 1 commit into from
Oct 6, 2017
Merged

Add stdin function #44

merged 1 commit into from
Oct 6, 2017

Conversation

sevagh
Copy link
Contributor

@sevagh sevagh commented Oct 5, 2017

Allows you to put the contents of a String in the program's stdin.

Fixes #26

@epage
Copy link
Collaborator

epage commented Oct 5, 2017

Looks great!

Two things

  • Unsure about @killercup but I try to keep the commit history clean in my projects. Unless killercup comes in and says otherwise, do you mind cleaning up the commits?
  • Name. As I said in Piping (stdin) to the process #26, I personally do not (yet) have opinions on the name (with_stdin, stdin, given_input). Not sure if @killercup does. I'd be willing to submit this as-is and release it, as long as the name is settled before 1.0 (Future of assert_cli #41).

@killercup
Copy link
Collaborator

Unsure about @killercup but I try to keep the commit history clean in my projects. Unless killercup comes in and says otherwise, do you mind cleaning up the commits?

@killercup agrees and is in favor of squashes.

Name.

Hmmm. Do we have an existing pattern? Do the API guidelines give us a pattern to use?

@sevagh
Copy link
Contributor Author

sevagh commented Oct 5, 2017

I'll squash the commits. I was more curious about other feedback, i.e. is a String OK? I tried something with a Read trait but that involves either boxes or lifetimes and the change became too intrusive.

@epage
Copy link
Collaborator

epage commented Oct 5, 2017

I'm not seeing anything in the naming guidelines that might apply.

Command configures through

  • stdin
  • `stdout
  • stderr

For assert_cli, we cuerrently have

  • stdout
  • stderr

So if we consider that precedence, then the name would be just stdin. Overall, I'm not seeing a big deciding factor in any direction.

This commit adds an stdin() function to supply stdin to the function.
@sevagh
Copy link
Contributor Author

sevagh commented Oct 5, 2017

I renamed it to stdin() and squashed the commits.

@sevagh
Copy link
Contributor Author

sevagh commented Oct 5, 2017

Also not sure if I should be running cargo fmt - it makes a lot of changes in files I haven't touched.

@epage epage changed the title Add with_stdin function Add stdin function Oct 6, 2017
@epage epage merged commit 875c4b1 into assert-rs:master Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants