Skip to content

RFC: structured rendering and switching to ammonite's color palette. #17624

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mpollmeier
Copy link
Contributor

@mpollmeier mpollmeier commented May 30, 2023

The current REPL's output rendering is suboptimal:

  1. the color highlighting is based on the string representation, i.e. all type information is thrown away before we render the results. I.e. information such as product labels, type information etc. are lost.

  2. The output isn't formatted for readability as users - ammonite does a much better job and also has a color palette that most users prefer

  3. there's a few bugs, e.g. REPL gives stack overflow during too eager syntax highlighting, e.g. when multiplying string #16904

This attempts to fix the points above by using pprint for the rendering, while we still have instances rather than just the output string.

Current stock repl: no product labels, no formatting, different colors
grafik

This RFC:
grafik

pprint brings in three additional jars weighing 360k in total.

This is just an RFC to gather feedback - an incomplete list of things TBD:

  • discuss & adjust truncation and maxHeight behaviour
  • do we really want Ammonite's color palette?
  • get the fansi patch into upstream fansi (see TODO)
  • tests
  • subproject composition: it probably makes sense to pull out a repl subproject to isolate it from scala3-compiler, e.g. to use the repl-only dependencies (jline, pprint) only for the REPL

Context: I'm using https://github.com/mpollmeier/scala-repl-pp as breeding ground for enhancing the REPL. The idea is to not create a hard fork but to bring the parts that are useful for dotty back upstream.

Looking forward to your feedback!

* pprint dependencyTree: 3 jars weighing 360k in total
  * pprint 0.8.1:  150k
  * fansi 0.4.0: 102k
  * sourcecode 0.3.0: 108k
  * if it's too large, workaround could be to allow injecting something by configuration - it's using reflection anyway...

* might make sense to factor repl out into a separate subproject... jline is probably also only needed for the repl, no?
@TheElectronWill
Copy link
Contributor

do we really want Ammonite's color palette?

My two cents about the colour palette: I prefer the current one :)

  • In ammonite's palette, the prompt is in magenta, which attracts the eye and distracts from the actual content, i.e. the expression that I'm writing. IMO the prompt should be less visible than the expression. As a user, I know that i'm writing Scala, I don't need to be reminded of it with so much intensity.
  • In the "new" screenshot, comments have lost their color (though that might be a bug, because I see that CommentColor is still set to BLUE)
  • Using a different color for types and for literals makes sense to me

That said, highlighting types like you did looks good!

@jchyb jchyb requested a review from prolativ June 5, 2023 12:12
@prolativ
Copy link
Contributor

prolativ commented Jun 6, 2023

Adding pprint as a dependency seems like quite a big deal actually.

First of all, there's a general rule that the compiler itself must not have any dependencies on external scala projects. That would indeed require that the REPL gets moved to a separate module. But that might require a minor release (@Kordyjan please confirm) because it would be published as a separate artifact depending on scala3-compiler.

Secondly, it seems that adding a transitive dependency on a particular version of fansi breaks some community build tests (Quill @ CommunityBuildC as I see) and the REPL should definitely work correctly if a user starts it with an explicit dependency on pprint or fansi

@mpollmeier
Copy link
Contributor Author

The rule "the compiler itself must not have any dependencies on external scala projects" makes a lot of sense but is unfortunately already violated: see the scala3-compiler-3.3.0.pom which includes three jline dependencies which are only needed for the REPL.

So, as stated in the description, we should separate out the repl as a subproject, which is actually a task that can be done separately from this proposal.

@mpollmeier
Copy link
Contributor Author

Good point that we might be breaking some existing builds by bringing in pprint and fansi: it'll hit those that rely on a different (incompatible) version of these libs. I didn't investigate how much code is actually used here, but I'm assuming it's not a lot - would it be ok to copy the relevant parts and move them to the dotty.tools.repl.pprint namespace to avoid the shadowing issue?

License-wise that should be fine (it's MIT licensed), but I want to be very careful to not step onto anyones toes, so I'd kindly ask the maintainers. It also means that we have a larger codebase to maintain, e.g. future patches need to be reintegrated etc.

Overall I'd say the benefits outweigh the costs though... thoughts?

@sjrd
Copy link
Member

sjrd commented Jun 13, 2023

The rule "the compiler itself must not have any dependencies on external scala projects" makes a lot of sense but is unfortunately already violated: see the scala3-compiler-3.3.0.pom which includes three jline dependencies which are only needed for the REPL.

Java dependencies are fine. What we really need to avoid are Scala dependencies.

@som-snytt
Copy link
Contributor

The repl calls into scala 2 runtime for stringifying.

I tried to improve semi-structured rendering for years, and I don't understand the resistance to change.

Even the more recent effort to just doing ellipsis correctly to show truncation stalled.

I would recommend improved decoupling.

Those PRs were
scala/scala#10180
scala/scala#8885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants