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

Expose OutStream rather than StdStream in Env #2463

Merged
merged 2 commits into from
Jan 14, 2018

Conversation

sgebbie
Copy link
Contributor

@sgebbie sgebbie commented Jan 4, 2018

By making Env expose and rely on the OutStream interface it becomes
possible to create mock environments for testing. For example one can
implement an OutStreamStringAccumulator that accumulates output to a
String that can then be checked in a unit test.

Currently the 'create()' method is provided in order to build an
'artificial' environment. This simply extends this ability to support
providing alternative 'out' and 'err' stream implementations.

Stewart Gebbie added 2 commits January 4, 2018 10:55
By making Env expose and rely on the OutStream interface it becomes
possible to create mock environments for testing. For example one can
implement an OutStreamStringAccumulator that accumulates output to a
String that can then be checked in a unit test.

Currently the 'create()' method is provided in order to build an
'artificial' environment. This simply extends this ability to support
providing alternative 'out' and 'err' stream implementations.
The examples currently used StdStream, but really they only depend on
the interface defined in OutStream.
@jemc
Copy link
Member

jemc commented Jan 4, 2018

It's arguable about whether this change should go through the RFC process.

I'm tempted to say it works as a "principle of least surprise bug", where you'd expect this API to expose this interface rather than the concrete type, because the intent of allowing to create artificial environments is already there.

However, before I merge, I'd like to give a chance for other folks to object to that and ask this to go through RFC. I'm marking this as "needs discussion during sync" so we can plan to discuss it on Wednesday but we may resolve asynchronously ahead of time.

@jemc jemc added changelog - changed Automatically add "Changed" CHANGELOG entry on merge needs discussion during sync labels Jan 4, 2018
@SeanTAllen
Copy link
Member

We are good to merge this AFTER 0.21.3 has been released.

Temporarily marking as DO NOT MERGE.

Thanks @sgebbie

@SeanTAllen SeanTAllen added do not merge This PR should not be merged at this time and removed needs discussion during sync labels Jan 10, 2018
@sgebbie
Copy link
Contributor Author

sgebbie commented Jan 11, 2018

That's great. Thanks for the update.

@SeanTAllen
Copy link
Member

@sgebbie i'm doing the 0.21.3 release tomorrow. too exhausted to do it today.

@SeanTAllen
Copy link
Member

0.21.3 has been released. merging. thanks @sgebbie.

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label Jan 14, 2018
@SeanTAllen SeanTAllen merged commit 514d9c1 into ponylang:master Jan 14, 2018
ponylang-main added a commit that referenced this pull request Jan 14, 2018
@SeanTAllen SeanTAllen mentioned this pull request May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants