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
REPL: upgrade to JLine 3 (benefits include multi-line editing) #8036
Conversation
This comment has been minimized.
This comment has been minimized.
I salivate every time I run across this in the PR queue. |
Took care of one tiny little paper cut in the macro-annot tests. Sorry, I didn't mean to add myself as the committer for just a one line change, but don't know how to convey this to git. |
c9f4bf7
to
10260be
Compare
I actually have a delta to push for this. |
Ok, I will hold off now that I have a ✅ |
Actually, one more commit for the Reader header. |
Actually, that last force was it, so have at it. I have a sense that there are several things to do, but I didn't make a checklist. |
I believe in fact you did, though in a different lifetime, as it were. |
I should add that I've been trying the REPL from this PR -- looks ready to ship in 2.13.1 to me! Really, really nice work. Multi-line history ftw! |
We are pretty committed to the current repl for the 2 series. It’s really
great to be on jline 3! I was a little too eager with my parse error
suppression in the last commit (there was an issue with lines with a
leading dot). It can probably just be dropped and refined later. I’m
currently on 👶leave, but the plan is to implement a few of the ideas
pioneered by ammonite in our own repl this summer.
…On Thu, 27 Jun 2019 at 22:15, som-snytt ***@***.***> wrote:
Thanks, @adriaanm <https://github.com/adriaanm>. I really ran out of
steam on this, sorry; maybe because not knowing if it would benefit anyone.
There are a few conflicting signals about what's envisioned for either
dotty or ammonite. Personally and professionally, I totes do use REPL as an
internal tooling interface, i.e., as a command line tool. I'm willing to
contribute further baby steps.
Speaking of ✅ today's nytimes crossword has answers like "checks and
balances" where the joke is that "check" is a single square. The "revealer"
is "check all the boxes". I noted in the comments that it would be nice if
the crossword supported Unicode. It would be nice if all our tools
supported Unicode.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8036?email_source=notifications&email_token=AAAWHS6VVRJ2TW35TFEBQ3LP4UNXNA5CNFSM4HK5YVZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYHZSY#issuecomment-506494155>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAWHSZG4FPMD4PBHXNYYIDP4UNXNANCNFSM4HK5YVZQ>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just get this in so that it gets tested by those working on 2.13.x. Looking through the change at a high level, i only have one question, about the dependencies.
I guess Adriaan is in 👨 👶 🌫️, as in Karl the fog not as in head in the clouds, which also explains his heady enthusiasm, though we'd have to run some benchmarks to accurately compare to his previous levels. I'll check out the previous commit and achieve minimal merge polish, with confidence that only @dwijnand is likely to complain about breakage. |
What breakage am I likely to complain about? The only one that comes to mind is source-breakage of zinc's "console" abstraction of the REPL, which we'd have to address. Other than that I don't have any PRs touching the same files (merge conflict breakage) or PRs looking to make changes to any behaviour here (semantics breakage). Did you mean the Zinc thing or am I forgetting something? |
Viewing my previous comment in Windows, I don't especially like 🌫 but maybe I'd like it more as a moustache. Sorry @dwijnand let me add that adding your handle in FIrefox on Windows was painful, which was the whole reason we fought the browser wars. Doubly sorry, now I don't remember what I was responding to. Please take my word that it was humorous to at least one of us, and that if I see a real issue in future I will report it. |
@SethTisue AFAICT the last action item before this can go into 2.13.1 is to see if we can restrict our additional dependencies to just jansi, and drop jna. Even with my brain 🌩 starting to clear, I still think we should get this in 😁 |
Tried it out. Did Jline completion always tab through alternatives? Some ordinary completion is broken, however, and will need repair. (Actually I couldn't reproduce: I guess the
Also, manage jline logging and this condition:
|
rebased and partially squashed. if all the commits pass CI, I think we could go ahead and merge this so that we'll all be testing it (and then address remaining blockers in followup PRs) |
use a different filename than we used with JLine 2, since the format is different (has timestamps) and because old files might have a different character encoding, we encountered that in testing also bring back code that restricts permissions on history file, this went missing during the upgrade
based on review feedback from hvesalai
I think this is too breaking, at least for JLine, but IMO also for JNA. The dependency on JLine is not optional, as we get
The dependency for JNA is kind of optional. The REPL works fine without JNA on macOS after adding |
Goal: avoid conflicting with sbt's own Jansi version Ref scala/scala-dev#678 https://github.com/jline/jline3#jansi-vs-jna says > On the Windows platform, relying on native calls is mandatory, so > you need to have either Jansi or JNA library in your classpath along > with the `jline-terminal-jansi` or `jline-terminal-jna` jar. When I tested on Windows 7 or Windows 10 using `sbt console` I got "WARNING: Unable to create a system terminal, creating a dumb terminal (enable debug logging for more information)" and key input also didn't work. (This could be more to do with sbt). I am guessing that Jansi used by sbt itself maybe interfering with Jansi that gets loaded from the compiler. Normally these duplicates are resolved by creating a sandbox ClassLoader, but maybe it doesn't exactly work for native libraries loaded into the same process. additional changes by Seth: * change Jansi -> JNA in legal info, Intellij config Co-authored-by: Seth Tisue <seth@tisue.net>
Okay, I've changed it back to regular dependencies, as the conservative choice, in the interest of getting this merged. (Eugene, if you think one or both dependencies should be optional, you could PR that separately and argue for it.) We could consider it (we already consider it!) future work to move the whole REPL front end into its own JAR, and then put the JLine and JNA dependencies on that (instead of scala-compiler.jar). |
merging, based on Lukas's re-approval today remaining blockers for 2.13.2 |
If we are ok with I'm fine with that too. For the tooling maintainers, it would be good to document the full dependency JARs (both for 2.13.2 and 2.13.1) somewhere before we forget all this. |
Forget what? 😺 |
To the PR description above, I have now added:
I'm not sure what else you might have in mind that we should document? |
When i try to use abstract class D { val d: Any; type T }
class DDep[D2 <: (D { type T = d.type }) { val d: Int }] {
def test_==(d: D2)(t: d.d.type): Boolean = d.d == t
} it formats pretty strangely: scala> :paste
// Entering paste mode (ctrl-D to finish)
abstract class D { val d: Any; type T }
|
| class DDep[D2 <: (D { type T = d.type }) { val d: Int }] {
| def test_==(d: D2)(t: d.d.type): Boolean = d.d == t
| } |
I was thinking somewhere more permanent like readme or scala-lang site, describing why/when JNA is needed. |
@bishabosha there was a "to do" for migrating the old line continuation code, but I can't find a ticket. Probably it's better to create an issue than comment here. (Some tickets are on scala-dev.) |
@bishabosha thanks for the report; opened scala/scala-dev#703 and marked it "blocker" @som-snytt you're probably thinking of scala/scala-dev#689 ("Excise old continuation handling" — I'd assumed that was just cleanup, but now we have a case where the old code is actually interfering) |
@eed3si9n suggestion recorded at https://github.com/scala/scala-dev/issues/685 |
JLine 3 supports multi-line editing, a better tab-completion UI, and more.
Configure keybindings with
-Xjline:emacs
(the default) or-Xjline:vi
; disable with-Xjline:off
History file is now
~/.scala_history_jline3
scala-compiler.jar
now depends onnet.java.dev.jna#jna
(instead oforg.fusesource.jansi#jansi
, as formerly).Minimal changes to support new API, with basic completion and history.
Fixes scala/bug#11367. Rebased from #7645