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

Implement initial and cleanup commands as well as value bindings for REPL #3007

Closed
felixmulder opened this Issue Aug 22, 2017 · 10 comments

Comments

Projects
None yet
5 participants
@Rajesh-Veeranki

This comment has been minimized.

Show comment
Hide comment
@Rajesh-Veeranki

Rajesh-Veeranki Aug 29, 2017

While I was working on this, few doubts popped up.

  1. Can we assume initial and cleanup commands are always parsed successfully?
    -- If yes, probably we can make case class Silent extends Parsed
  2. If not, is the case class Silent always one of Parsed, SyntaxErrors, Newline, SigKill, Command?
    Hope I made sense.

Rajesh-Veeranki commented Aug 29, 2017

While I was working on this, few doubts popped up.

  1. Can we assume initial and cleanup commands are always parsed successfully?
    -- If yes, probably we can make case class Silent extends Parsed
  2. If not, is the case class Silent always one of Parsed, SyntaxErrors, Newline, SigKill, Command?
    Hope I made sense.
@felixmulder

This comment has been minimized.

Show comment
Hide comment
@felixmulder

felixmulder Aug 30, 2017

Contributor

You're making sense :)

  1. No
  2. Yes

One way to do this is to say:

case class Silent(underlying: ParseResult) extends ParseResult {
  require(!underlying.isInstanceOf[Silent])
}

that way we can unpack it to the underlying result.

A more type safe way to do this is to introduce a marker trait extending ParseResult that denotes something concrete e.g:

          ParseResult
               |
   +-----------+-----------+
   |                       |
Silent             ConcreteParseResult
                           |
                           +--------- Parsed, SyntaxErrors, NewLine etc

And then making Silent be:

case class Silent(underlying: ConcreteParseResult) extends ParseResult

Perhaps there's a better name than concrete though 😄

Contributor

felixmulder commented Aug 30, 2017

You're making sense :)

  1. No
  2. Yes

One way to do this is to say:

case class Silent(underlying: ParseResult) extends ParseResult {
  require(!underlying.isInstanceOf[Silent])
}

that way we can unpack it to the underlying result.

A more type safe way to do this is to introduce a marker trait extending ParseResult that denotes something concrete e.g:

          ParseResult
               |
   +-----------+-----------+
   |                       |
Silent             ConcreteParseResult
                           |
                           +--------- Parsed, SyntaxErrors, NewLine etc

And then making Silent be:

case class Silent(underlying: ConcreteParseResult) extends ParseResult

Perhaps there's a better name than concrete though 😄

@Rajesh-Veeranki

This comment has been minimized.

Show comment
Hide comment
@Rajesh-Veeranki

Rajesh-Veeranki Aug 31, 2017

Thanks for the help. I've submitted a WIP PR, to check if I'm going in right direction. If I understood marker traits correctly, I need to declare a trait, say Concrete and attach it to all Parsed, SyntaxErrors, NewLine etc right?
Say,
case class Parsed(...) extends ParseResult with Concrete

Also I'm getting this warning, even though I am unpacking the inner object and required Silent can't inherit Silent
[warn] It would fail on the following input: Silent(_) [warn] parseResult match {

Rajesh-Veeranki commented Aug 31, 2017

Thanks for the help. I've submitted a WIP PR, to check if I'm going in right direction. If I understood marker traits correctly, I need to declare a trait, say Concrete and attach it to all Parsed, SyntaxErrors, NewLine etc right?
Say,
case class Parsed(...) extends ParseResult with Concrete

Also I'm getting this warning, even though I am unpacking the inner object and required Silent can't inherit Silent
[warn] It would fail on the following input: Silent(_) [warn] parseResult match {

@SzymonPajzert

This comment has been minimized.

Show comment
Hide comment
@SzymonPajzert

SzymonPajzert Aug 5, 2018

Contributor

Is this feature still requested? As I understand, it wasn't implemented in the master in the end.

Contributor

SzymonPajzert commented Aug 5, 2018

Is this feature still requested? As I understand, it wasn't implemented in the master in the end.

@Blaisorblade

This comment has been minimized.

Show comment
Hide comment
@Blaisorblade

Blaisorblade Aug 5, 2018

Contributor

Is this feature still requested?

Yes.

As I understand, it wasn't implemented in the master in the end.

We didn't stop liking the feature. The help wanted label means "outside contributions welcome" ;-).

Contributor

Blaisorblade commented Aug 5, 2018

Is this feature still requested?

Yes.

As I understand, it wasn't implemented in the master in the end.

We didn't stop liking the feature. The help wanted label means "outside contributions welcome" ;-).

@SzymonPajzert

This comment has been minimized.

Show comment
Hide comment
@SzymonPajzert

SzymonPajzert Aug 12, 2018

Contributor

I have implemented the changes, they seem to be much easier after refactoring, but I haven't added yet any tests to check the results. Where should I put them?

Contributor

SzymonPajzert commented Aug 12, 2018

I have implemented the changes, they seem to be much easier after refactoring, but I haven't added yet any tests to check the results. Where should I put them?

@Blaisorblade

This comment has been minimized.

Show comment
Hide comment
@Blaisorblade

Blaisorblade Aug 13, 2018

Contributor

Under sbt-dotty/sbt-test/ we have tests that use sbt-scripted (https://www.scala-sbt.org/1.0/docs/Testing-sbt-plugins.html); sbt-dotty/sbt-test/sbt-dotty/example-project/test seems closest, and 061fdb1 and f503ad3 seem about it (but I'm not sure on the status, and I think @allanrenucci is on vacation).
EDIT: but while we wait, feel free to experiment with them!

Contributor

Blaisorblade commented Aug 13, 2018

Under sbt-dotty/sbt-test/ we have tests that use sbt-scripted (https://www.scala-sbt.org/1.0/docs/Testing-sbt-plugins.html); sbt-dotty/sbt-test/sbt-dotty/example-project/test seems closest, and 061fdb1 and f503ad3 seem about it (but I'm not sure on the status, and I think @allanrenucci is on vacation).
EDIT: but while we wait, feel free to experiment with them!

@allanrenucci

This comment has been minimized.

Show comment
Hide comment
@allanrenucci

allanrenucci Aug 20, 2018

Contributor

For reference, here is where it is done for scalac.

initialCommands and cleanupCommands are not interpreted in a silent mode. If a command in initialCommands fails, the REPL exits with "Interpreter encountered errors during initialization!"

Only value binding should run in a silent mode

Contributor

allanrenucci commented Aug 20, 2018

For reference, here is where it is done for scalac.

initialCommands and cleanupCommands are not interpreted in a silent mode. If a command in initialCommands fails, the REPL exits with "Interpreter encountered errors during initialization!"

Only value binding should run in a silent mode

@SzymonPajzert

This comment has been minimized.

Show comment
Hide comment
@SzymonPajzert

SzymonPajzert Aug 26, 2018

Contributor

@allanrenucci, could you please also tell me about the status of testing of the feature? I tried to uncomment console in sbt-dotty/sbt-test/sbt-dotty/example-project/test, but received: java.lang.IllegalStateException: Unable to create a system terminal

Contributor

SzymonPajzert commented Aug 26, 2018

@allanrenucci, could you please also tell me about the status of testing of the feature? I tried to uncomment console in sbt-dotty/sbt-test/sbt-dotty/example-project/test, but received: java.lang.IllegalStateException: Unable to create a system terminal

@Blaisorblade

This comment has been minimized.

Show comment
Hide comment
@Blaisorblade

Blaisorblade Aug 26, 2018

Contributor

@SzymonPajzert Yeah that doesn't work on CI, there's a fix at #5018. EDIT: looking at #4933, it looks like #5018 is enough to fix it.

Contributor

Blaisorblade commented Aug 26, 2018

@SzymonPajzert Yeah that doesn't work on CI, there's a fix at #5018. EDIT: looking at #4933, it looks like #5018 is enough to fix it.

allanrenucci added a commit that referenced this issue Sep 2, 2018

Merge pull request #4933 from SzymonPajzert/wip/silencing-output-i3007
Fix #3007: Add support for initial and cleanup commands in the REPL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment