Skip to content
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

Accept defs in REPL #1262

Merged
merged 2 commits into from
May 2, 2021
Merged

Accept defs in REPL #1262

merged 2 commits into from
May 2, 2021

Conversation

basile-henry
Copy link
Contributor

Corresponding issue: #1187

Accept defs in REPL by parsing the input as part of the validation (on Enter). This is a reliable way to determine if the input so far could become a valid expression with more input lines.

Currently no new prompt is presented on input newline. The issue mentions a prompt, is this something I should try to get back?

basile-henry and others added 2 commits May 2, 2021 22:31
by parsing the input as part of the validation (on Enter). This is a
reliable way to determine if the input so far could become a valid
expression with more input lines.
Copy link
Sponsor Contributor

@rtfeldman rtfeldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is definitely a new record - fastest time between joining the repo to having a first PR up! 🏆

Congrats, and thanks so much for the first contribution! 😃

@rtfeldman rtfeldman merged commit 1e1c3f4 into trunk May 2, 2021
@rtfeldman rtfeldman deleted the basile/defs-in-repl branch May 2, 2021 21:14
@@ -71,7 +71,15 @@ impl Validator for InputValidator {
if ctx.input().is_empty() {
Ok(ValidationResult::Incomplete)
} else {
Ok(ValidationResult::Valid(None))
let arena = bumpalo::Bump::new();
match roc_parse::test_helpers::parse_expr_with(&arena, ctx.input()) {
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but for a future PR, it would be good to extract this into a separate function so we aren't depending on test_helpers inside production code. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes fair enough! I saw it as part of the public API of the crate, but the name should have hinted not to use it. Maybe we could rename that module, because it seems like a useful utility to have in order to parse without too much setup.

@rtfeldman
Copy link
Sponsor Contributor

rtfeldman commented May 2, 2021

Currently no new prompt is presented on input newline. The issue mentions a … prompt, is this something I should try to get back?

That would be lovely!

The way we used to do it (before a rewrite of some combination of the parser and the REPL which lost it) was that when we were in "multiline mode" we would render instead of » for the REPL prompt. (Currently we render no prompt on the additional lines.) This was especially nice because indentation matters for defs, so it was reassuring to both see things line up, as well as to see a different character from the usual » so it was clear we were in a different input mode.

So today after merging this PR, it looks like this:

» x = 5
y = 6

x + y

11 : Num *

...but it would be nice to have it look like this instead:

» x = 5
… y = 6
… 
… x + y
… 
11 : Num *

@rtfeldman
Copy link
Sponsor Contributor

If you're feeling ambitiuous, here's something that we never had before, but which would make it even nicer: after finally accepting the input, "backspace two spaces" to erase the trailing and the space after it.

That would make the final output look like this:

» x = 5
… y = 6
… 
… x + y
11 : Num *

So if you'd like to go for a stretch goal, that would be a nice one!

@basile-henry
Copy link
Contributor Author

when we were in "multiline mode" we would render … instead of » for the REPL prompt

Were you using the Validator part of rustyline for multiline support or were you rolling your own multiline on top of rustyline's single line inputs? I couldn't find anything in rustyline to set a "continuation" prompt. Maybe something I could work on. 😄

I agree that having a "continuation" prompt to keep alignment is necessary. I have kept the logic that let the user input an empty line though, so you can still do this at the moment:

»
x = 5
y = 6

x + y

11 : Num *

@rtfeldman
Copy link
Sponsor Contributor

We were rolling our own - in fact, I think adding rustyline was exactly when the continuation prompt went away! 😄

@basile-henry
Copy link
Contributor Author

I have had a look at various issues and PRs on rustlyline on the subject of continuation prompts. This PR kkawakam/rustyline#372 looks promising.

I have quickly implemented multiline prompt using it: https://github.com/rtfeldman/roc/tree/basile/multiline-prompt

It looks like this:


  The rockin’ roc repl
────────────────────────

Enter an expression, or :help, or :exit/:q.

» x = 2
… y = 3
… x + y

5 : Num *

»
CTRL-C

Unfortunately it has been open for over a year now. Maybe we should mention we're interested. 😄

@rtfeldman
Copy link
Sponsor Contributor

Aha, nice!

Honestly, I'd be fine with our using a fork of rustyline that adds the fix. I just forked onto https://github.com/rtfeldman/rustyline (the prompt-fix tag has the fix), so we can import it with:

rustyline = { git = "https://github.com/rtfeldman/rustyline", tag = "prompt-fix" }

It'd be great if it got merged upstream, of course, but I don't know if we've ever bothered to update rustyline because the way we use it is so basic. 😄

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.

3 participants