Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@seandgrimes
Copy link
Contributor

Added new IRepl interface that inherits from IScriptExecutor and adds a new method named Quit

Updated the Execute method of IRepl to accept an instance of IRepl instead of IScriptExecutor

Updated Repl to implement IRepl and the new Quit method, which calls Terminate() and Environment.Exit(0)

Updated the Execute() method of the existing REPL commands to accept an instance of IRepl instead of IScriptExecutor

Added a new ExitCommand that uses the new Repl.Quit() method to exit the REPL

Updated RuntimeServices to bind IRepl to Repl

Updated existing REPL command tests to pass in a mock of IRepl to IReplCommand.Execute instead of a mock of IScriptExecutor

Added new tests to cover the new ExitCommand

Added new IRepl interface that inherits from IScriptExecutor and adds a new method named Quit

Updated the Execute method of IRepl to accept an instance of IRepl instead of IScriptExecutor

Updated Repl to implement IRepl and the new Quit method, which calls Terminate() and Environment.Exit(0)

Updated the Execute() method of the existing REPL commands to accept an instance of IRepl instead of IScriptExecutor

Added a new ExitCommand that uses the new Repl.Quit() method to exit the REPL

Updated RuntimeServices to bind IRepl to Repl

Updated existing REPL command tests to pass in a mock of IRepl to IReplCommand.Execute instead of a mock of IScriptExecutor

Added new tests to cover the new ExitCommand
Removing "Contracts" from "Contracts.IRepl" from updated tests
The PromptsUserBeforeExiting() unit test was eternal looping since the
Console.ReadLine() method was never stubbed to return a value
public void Quit()
{
Terminate();
Environment.Exit(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment.Exit should be moved into the IConsole abstraction - there is no guarantee which console is in use, so exiting the REPL might be different based on those

@filipw
Copy link
Member

filipw commented Aug 23, 2014

very good PR, thanks a lot.

Creating IRepl has been on the todo list for quite a while (see #434) so I'm very happy that you got the ball rolling. You should also inject IRepl through the constructor of ExecuteReplCommand (it's currently newed up directly there and now we have an opportunity to get rid of that).

Changing the signature on IReplCommand is a breaking change, but since we are pre-1.0 it's fine, especially as it's totally justifiable here.

I also left some comments inline.

@seandgrimes
Copy link
Contributor Author

Hi Filip, thank you for taking the time to look at my pull request.

I have a question about #434, by "inject" do you mean move the construction of the Repl object to CommandFactory and pass it to the ExecuteReplCommand constructor as a parameter?

Also, would it be appropriate to have an IRepl.Quit() method that would proxy to the IConsole.Quit() method?

@filipw
Copy link
Member

filipw commented Sep 4, 2014

sorry I was sure I replied earlier!
the answers are yes and yes.

@adamralph
Copy link
Contributor

#427

@filipw
Copy link
Member

filipw commented Dec 23, 2014

@munumafia really sorry for leaving this out there for that long. If you notice, we have merged a bunch of changes related to REPL infrastructure and the commands themselves. If you are still up for the issue, can you please rebase? thanks

@filipw filipw mentioned this pull request Mar 6, 2015
@filipw
Copy link
Member

filipw commented Mar 6, 2015

superseded by #959. @munumafia I rebased your branch so that your commits show up in the history

@filipw filipw closed this Mar 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.