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

Made Evaluator a trait #14

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@alexarchambault
Collaborator

alexarchambault commented Feb 9, 2015

Hi,
I'm thinking about plugging some other evaluators on your repl. For that, this PR tries to abstract away the Evaluator API. Would you be ok with keeping a distinct Evaluator trait like this? Ideally, with no too low level methods on it, like evalClass and the like.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 9, 2015

Looks reasonable to me! Comments:

  • sbt "~repl/test-only -- ammonite.repl.EvaluatorTests" fails with your PR merged into master; it looks like the test-Checker now has color output which it doesn't expect
  • No need to put private everywhere. val eval: Evaluator = new ScalaEvaluator should be enough encapsulation for now, all private does is add noise. The public interface is already fully defined in Evaluator.
@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 9, 2015

Also, do you want to talk about what "other evaluators" means for you? It would be helpful to know so we don't work cross purposes. I have my own ideas for what "other evaluators" means =P

@alexarchambault

This comment has been minimized.

Collaborator

alexarchambault commented Feb 9, 2015

I made some bigger changes (abstracting Compiler and Preprocessor, and using them from Evaluator), any aspect of it can be discussed.

My goal would be to plug kernels from my project Jove, https://github.com/jove-sh/jove (it underwent major refactoring and modularization since last public commit, although it should work as is - I'll push these changes and make a first public announcement about it very soon). Hopefully, the spark interpreter from it should work straight away.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 9, 2015

Cool. I'm probably going to have to put off reviewing until tonight when I get back from work.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 10, 2015

Quick question: Why do you put the Compiler and Preprocessor into the Evaluator? I had very intentionally kept them separate and only cross-injected the things I needed

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 10, 2015

FWIW Splitting out the compiler, preprocessor and evaluator traits are uncontroversial and I can merge that separately if you want

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 10, 2015

I think I know why you wanted to stuff everything into Evaluator, and I think 9e0c1ae should satisfy some of those desires

@alexarchambault

This comment has been minimized.

Collaborator

alexarchambault commented Feb 10, 2015

Thanks, I'll have a closer look at the last changes, and will actually try to plug in my own interpreters one way or another, later today. Let's put this PR on hold. The new Interpreter looks interesting indeed.

@lihaoyi

This comment has been minimized.

Owner

lihaoyi commented Feb 14, 2015

Closing this for now

@lihaoyi lihaoyi closed this Feb 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment