Introduce ReplPrinter as a hook for alternatives to replStringOf #5222

dwijnand · 2016-06-09T07:34:45Z

Requires making the imports available during the eval phase of the REPL.

Review by @retronym.

dwijnand · 2016-06-09T08:03:49Z

It would be lovely if this could make M5 :-)

Also i expect the unwrapping of $print will start failing once the repl wrappers are gone (#5220).

Requires making the imports available during the eval phase of the REPL.

dwijnand · 2016-06-11T13:55:34Z

Ah didn't realise this had failed the tests. Updated test/files/run/repl-out-dir.check.

Also, review by @som-snytt?

som-snytt · 2016-06-11T18:19:08Z

The one everyone asks for is to quote strings. It might be nice to include a standard one for easy import from a suitable namespace, like ReplPrinter.Quoted.quoter so import ReplPrinter.Quoted._.

This isn't nuanced:

scala> "hi"
res0: String = hi

scala> import scala.tools.nsc.interpreter.ReplPrinter
import scala.tools.nsc.interpreter.ReplPrinter

scala> :pa
// Entering paste mode (ctrl-D to finish)

implicit val `quoted strings`: ReplPrinter[String] =
  new ReplPrinter[String] {
    def print(s: String, max: Int) =
      if (s.contains('"')) s"${"\""*3}$s${"\""*3}"
      else s""""$s""""
  }

// Exiting paste mode, now interpreting.

quoted strings: scala.tools.nsc.interpreter.ReplPrinter[String] = $anon$1@6ea246af

scala> res0
res1: String = "hi"

You quickly run into implicit management by name:

scala> implicit val `quoted strings`: ReplPrinter[String] = new ReplPrinter[String] {      
     | def print(s: String, max: Int) = s.toUpperCase
     | }
quoted strings: scala.tools.nsc.interpreter.ReplPrinter[String] = $anon$1@fd4459b

scala> res0
res2: String = HI

scala> implicit val upperly: ReplPrinter[String] = new ReplPrinter[String] {
     | def print(s: String, max: Int) = s.toUpperCase }
upperly: scala.tools.nsc.interpreter.ReplPrinter[String] = $anon$1@6200b644

scala> res0
<console>:19: error: ambiguous implicit values:
 both value quoted strings of type => scala.tools.nsc.interpreter.ReplPrinter[String]
 and value upperly of type => scala.tools.nsc.interpreter.ReplPrinter[String]
 match expected type scala.tools.nsc.interpreter.ReplPrinter[String]
 + "res3: String = " + _root_.scala.tools.nsc.interpreter.replStringOf(res3, 1000)
                                                                      ^

This is where smart imports is useful, either better automation or manually quarantine imports. The other ticket for spark has similar problem.

My only concern is how the object nesting in $eval affects spark, who introduced -Yrepl-class-based. Especially if all the imports are in play? That's what the other ticket is about, this PR #5210 for SI-9799.

Really, what's needed is a general way to specify the templating. For example, another requested feature is to emit val res1: String = "hi"so that the output line is code.

Since the concerns are out-of-scope, maybe just disable the change under the spark option; unless it's shown not to be a problem.

…ased

dwijnand · 2016-06-11T22:24:25Z

@som-snytt I see. I've revert the behaviour under -Yrepl-class-based. WDYT?

som-snytt · 2016-06-26T13:29:28Z

More ideas https://issues.scala-lang.org/browse/SI-9829 such as convert controls to escapes in color, CR -> \r or \u000d in red?

adriaanm · 2016-07-05T21:35:20Z

src/repl/scala/tools/nsc/interpreter/ReplPrinter.scala

+}
+
+object ReplPrinter {
+  implicit def default[A]: ReplPrinter[A] = new ReplPrinter[A] {


Could we have one instance, like so:

implicit object defaultReplPrinter extends ReplPrinter[Any] { def print(x: Any, maxElements: Int) = ScalaRunTime.replStringOf(x, maxElements) }

?

Yep, good eye. Done.

adriaanm · 2016-07-05T21:53:45Z

test/files/run/repl-printer-classbased.check

+scala> 
+
+scala> 23
+res1: Int = 23


I was expecting a "!" at the end here -- how come there's not?

Oh, this is the class-based one.

perhaps add a comment to the test case that says we don't enable custom string of under this mode

Everyone expects it to end with a bang.

I wasn't sure how to leave the comment, so I've opted for in the transcript.

This comment didn't fold because that line it's on didn't change, so, for clarity, I've added a commit that adds the comment.

som-snytt · 2016-07-05T22:50:35Z

I just returned from remote regions; I'll give this another look and try it out.

…based

dwijnand · 2016-07-09T10:58:32Z

Anything left here? This is a REPL-only change. Can it go in?

som-snytt · 2016-07-09T18:20:36Z

The templating part may conflict with #5220, for which I have a test to fix still. I'll try to nail that down this weekend, and try this on top of it.

lrytz · 2016-07-13T09:22:28Z

test/files/run/repl-out-dir.check

@@ -11,13 +11,17 @@ repl-out-dir-run.obj
        $eval$.class
        $eval.class
    $line2
+        $eval$$iw$$iw$.class
+        $eval$$iw$.class


does this mean every repl line generates two additional classfiles? is there a drawback to this? (performance?)

Yeah it does. I'm not sure what drawbacks there might be, but I avoided it for Spark-reasons.

lrytz · 2016-07-13T09:23:32Z

I think it would be weird to have such a mechanism work only in one of object-based / class-based, there does not seem to be any inherent relation between the two concerns.

dwijnand · 2016-07-13T09:50:58Z

Which two concerns do you mean? I removed it for class-based because @som-snytt was concerned about the performance impact for Spark.

lrytz · 2016-07-13T10:02:47Z

i just think it doesn't make sense for a feature like the one you're implementing to be dependent on some internal representation within the repl.

dwijnand · 2016-07-13T12:05:53Z

Yeah it's not ideal, but it's a compromise I personally was fine with.

How do we go about proving that this doesn't affect Spark?

Alternatively how do you feel about an opt-out option, for if it proves to be a problem?

som-snytt · 2016-07-13T17:17:19Z

Also, sorry my weekend went to understanding how imports work. I still intend to help with the concerns.

adriaanm · 2016-08-02T18:44:37Z

Could we do a bit of a cost/benefit analysis of this and #5220, as well as a risk assessment of causing a regression in the repl, which would lengthen the RC cycle? I'm tempted to push this to 2.12.1 so that we can consider this together with #5220 and go a bit deeper in dealing with some templating technical debt, rather than taking on more.

dwijnand · 2016-08-02T19:12:14Z

I don't like the idea of "shackling" together two pull requests which are doing distinct thing, even if they're in the same area of the codebase.

Why are you saying that this adds technical debt?

adriaanm · 2016-08-02T19:37:56Z

I don't think it's unreasonable to consider these PRs together, as they touch very similar parts of the REPL. Having to disable this for the class-based templates seems to imply we're going to have to come back to this and make it work there too, no?

adriaanm · 2016-08-02T19:44:33Z

I also would like to see a more technical response to @lrytz's concern. Everything is a compromise, but what are we conceding and what for? How would we solve this if we had more time? The REPL encoding is already very complicated, so any change is taking on technical debt for a nicer way of implementing the REPL.

som-snytt · 2016-08-02T19:58:39Z

Sorry, I meant to devote time to this, but the day job got busy. Time flies. Also, it slipped my mind. Possibly I was hoping it was fixed in ammonite.

adriaanm · 2016-08-09T21:38:01Z

For the reasons stated above, and with this not being a release blocker, I'm re-assigning this to 2.12.1. It would be great to have a concerted effort in cleaning up the REPL encoding once 2.12.0 has shipped.

dwijnand · 2016-08-10T15:03:04Z

I would be happy help clean up the REPL (but I would need some guidance as I have no prior experience of REPL implementations to compare this one against). I'm fine with this going in 2.12.1 rather than 2.12.0, but I would just like it one way or another.

Without doing a cost/benefit analysis of this and #5220, or a risk assessment, would you consider this feature being opt-in via a compiler flag? Or is that a no-go?

adriaanm · 2016-08-10T15:19:32Z

Thanks for understanding -- we're happy to help with this once 2.12.0 is out!

som-snytt · 2016-08-10T19:59:54Z

I've been using spark for day job; you know spark shell doesn't even inherit the improved welcome message in 2.11.8? I sense that there's something important to do for repl re-use and customizability for 2.12, but not sure if I'll find time to realize it.

FTR, there's an episode of "Inspector Morse" which turns on the OED recommendation of "-ize". (A note writer uses "-ise" inappropriately and misspells "desperate" like "separate".) (It was a suicide note, putatively, and this was a few years before the internet.) In hindsight, I would have used stronger language about "-optimise."

http://blog.oxforddictionaries.com/2011/03/ize-or-ise/
http://www.oxforddictionaries.com/words/ize-ise-or-yse

som-snytt · 2016-08-11T00:03:43Z

Because there is no ticket?, linking to https://issues.scala-lang.org/browse/SI-9829 about pretty printing against impossible odds.

adriaanm · 2016-12-01T01:47:29Z

would you consider this feature being opt-in via a compiler flag? Or is that a no-go?

sorry I missed this the first time -- I would prefer putting this under a flag, as I'm concerned this could slow down the REPL due to additional wrapping

lrytz · 2016-12-02T09:00:52Z

I still think we should come up with a solution that is also fine for -Yrepl-class-based / spark. IUC we'd also like to switch to this encoding by default at some point, and then the feature would go away.

dwijnand · 2016-12-02T09:16:41Z

If I put it behind a feature flag, the feature would be enabled under class-based too, IMO.

dwijnand · 2016-12-02T12:39:13Z

Clearly there's more work to do here for this to be still in the 2.12.1 queue.

But thanks for the recent feedback.

dwijnand · 2016-12-11T00:58:07Z

I don't have the stamina to take this to completion if it requires first delivering the "avoid $iw wrappers" change in the REPL.

som-snytt · 2016-12-11T02:15:55Z

There's pressure both to de-wrapperize and avoid object wrappers entirely and also do customizable printing. To which I'd add customizable wrapping. This won't go unused.

scala-jenkins added this to the 2.12.0-RC1 milestone Jun 9, 2016

Introduce ReplPrinter as a hook for alternatives to replStringOf

c3969fb

Requires making the imports available during the eval phase of the REPL.

dwijnand force-pushed the repl-printing branch from 22e2427 to c3969fb Compare June 11, 2016 13:54

Remove import wrapping & print result unwrapping under -Yrepl-class-b…

bcf91b7

…ased

adriaanm reviewed Jul 5, 2016
View reviewed changes

Make the default ReplPrinter an object

9533e80

adriaanm reviewed Jul 5, 2016
View reviewed changes

Leave a comment explaining ReplPrinter is ignored under -Yrepl-class-…

d571c52

…based

dwijnand force-pushed the repl-printing branch from f844d2f to d571c52 Compare July 8, 2016 00:56

lrytz reviewed Jul 13, 2016
View reviewed changes

adriaanm self-assigned this Aug 2, 2016

adriaanm assigned retronym Aug 2, 2016

adriaanm modified the milestones: 2.12.1, 2.12.0-RC1 Aug 9, 2016

som-snytt mentioned this pull request Nov 3, 2016

SI-10023: third iteration using member.keyword and special handling for parameterless methods #5498

Closed

smarter mentioned this pull request Dec 1, 2016

[REPL] Add show capability to common types scala/scala3#1761

Merged

adriaanm modified the milestones: 2.12.2, 2.12.1 Dec 2, 2016

dwijnand closed this Dec 11, 2016

dwijnand deleted the repl-printing branch December 11, 2016 00:58

SethTisue removed this from the 2.12.2 milestone Feb 22, 2017

This was referenced Apr 7, 2017

reading a file produces odd (corrupt?) type statements scala/bug#9829

Closed

Improve REPL display scala/bug#10024

Closed

dwijnand mentioned this pull request May 13, 2018

Fix #4415: remove show from repl scala/scala3#4437

Merged

Introduce ReplPrinter as a hook for alternatives to replStringOf #5222

Introduce ReplPrinter as a hook for alternatives to replStringOf #5222

Conversation

dwijnand commented Jun 9, 2016

dwijnand commented Jun 9, 2016 • edited Loading

dwijnand commented Jun 11, 2016

som-snytt commented Jun 11, 2016 • edited Loading

dwijnand commented Jun 11, 2016

som-snytt commented Jun 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

som-snytt commented Jul 5, 2016

dwijnand commented Jul 9, 2016

som-snytt commented Jul 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrytz commented Jul 13, 2016

dwijnand commented Jul 13, 2016

lrytz commented Jul 13, 2016

dwijnand commented Jul 13, 2016

som-snytt commented Jul 13, 2016

adriaanm commented Aug 2, 2016

dwijnand commented Aug 2, 2016

adriaanm commented Aug 2, 2016

adriaanm commented Aug 2, 2016

som-snytt commented Aug 2, 2016

adriaanm commented Aug 9, 2016

dwijnand commented Aug 10, 2016 • edited Loading

adriaanm commented Aug 10, 2016

som-snytt commented Aug 10, 2016

som-snytt commented Aug 11, 2016

adriaanm commented Dec 1, 2016 • edited Loading

lrytz commented Dec 2, 2016

dwijnand commented Dec 2, 2016

dwijnand commented Dec 2, 2016

dwijnand commented Dec 11, 2016

som-snytt commented Dec 11, 2016

dwijnand commented Jun 9, 2016 •

edited

Loading

som-snytt commented Jun 11, 2016 •

edited

Loading

dwijnand commented Aug 10, 2016 •

edited

Loading

adriaanm commented Dec 1, 2016 •

edited

Loading