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

Multi-line REPL #1867

Merged
merged 7 commits into from
Jun 21, 2020
Merged

Multi-line REPL #1867

merged 7 commits into from
Jun 21, 2020

Conversation

basile-henry
Copy link
Collaborator

The actual multi-line logic is implemented in the upcoming repline 0.4 release. This PR simply updates the REPL code to use this new release.

I wanted to open this draft PR to gather some feedback on the feature and see if there was anything missing. If/when the feedback is positive, and I'm sure no extra feature will be needed on the repline side of things, I will ask for a new release to be cut, and then update this PR to use the Hackage release.

To summarize the feature:

  • Multi-line input using the new :paste command
  • <Ctrl-D> to finish/submit the multi-line input
  • Multi-line commands are supported inside :paste (only one command per multi-line input)
  • Autocomplete works

@thebritican Does this resolve #1680? Reading the issue once again, I am not sure I understand exactly how :load and :save are used as part of some documentation. Are you giving to your readers a file to :load?
I think I need to have a closer look at the interaction of this multi-line input with the :load/:save feature, as the format is line based. Hopefully it's not completely broken and I can make it work. 🤞

];
testHaskellDepends = [ base containers mtl process ];
doHaddock = false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we're hitting a weird GHC 8.4.4 haddock bug:

Haddock coverage:

src/System/Console/Repline.hs:266:3: error:
    parse error on input ‘-- | banner function’
    |
266 |   -- | banner function
    |   ^^^^^^^^^^^^^^^^^^^^

At https://github.com/sdiehl/repline/blob/cfdbb815d4410db3bbebae3a53fd03328de50d48/src/System/Console/Repline.hs#L263-L267

This does not happen with GHC 8.6.5!

Copy link
Collaborator

@sjakobi sjakobi Jun 17, 2020

Choose a reason for hiding this comment

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

Looks like something that would be good to fix in time for 0.4!

Also good to check in CI…

EDIT: Reported in sdiehl/repline#31

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed upstream!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's broken again, it's originally an ormolu issue and it is marked as wontfix: tweag/ormolu#624

@gregziegan
Copy link

gregziegan commented Jun 18, 2020

I am not sure I understand exactly how :load and :save are used as part of some documentation

Let me clarify our general problem first, since it may help with future work.

Problem

I can't add a breakpoint or print statement in a dhall file.

This is important when reviewing and changing functions, not really when doing record operations.

Decent solution

Folks want to copy a file up to some let binding and paste it into a repl.

Problem with solution: loading things into a repl means I have to go add : in front of every let binding

:save / :load ?

We use :load to seed repl environments for documentation. It's a bit annoying to keep these up-to-date as the package we're documenting changes. We could automate this process with some hacky regexes in the meantime to add colons in front of lets.

If multiline paste improvements could eventually handle a serious of let expressions and bind each of them in the repl scope, repl seed files would be less necessary. I think improving the experience around :load/:save is another topic, though, since they have their own workflow complexities.

Mutli-line

This will help enormously for k8s examples as-is. This will help the seed files' readability (not one massively long line of minimal k8s config). And, this will help with getting more folks working in the repl with large expressions. Thank you!

@basile-henry
Copy link
Collaborator Author

@thebritican Thanks for the explanation!

The multi-line feature did end up breaking the :save/:load files, as they would get saved with the multi-line let bindings (when they remained multi-line after being normalized):

 dhall repl
Welcome to the Dhall v1.33.0 REPL! Type :help for more information.
 :paste
-- Entering multi-line mode. Press <Ctrl-D> to finish.
| :let simple-multi-line =
|   2 +
|   2
|

simple-multi-line : Natural

 simple-multi-line

4

 :paste
-- Entering multi-line mode. Press <Ctrl-D> to finish.
| :let string-multi-line = ''
|   Hello
|   World
|   ''
|

string-multi-line : Text

 string-multi-line

''
Hello
World
''

 :save test-file
Context saved to `test-file`
 cat test-file
:let simple-multi-line = 4
:let string-multi-line = ''
Hello
World
''

That was a problem for the :load function which expected line based inputs. Commit 809f400 adds support to load such a file correctly.

Does that completely solve your issue or do you think there is still something that would be needed in order to seed the environment that goes with the docs?

As a side note, I don't know how much you should be relying on the exact format of serialisation used by the Dhall REPL. There are no specs for it and it's very much a ad-hoc format at the moment 😅
But I'm sure if you specify which version of the Dhall binary they are intended for, it should be fine 👍

@gregziegan
Copy link

Does that completely solve your issue or do you think there is still something that would be needed in order to seed the environment that goes with the docs?

I'll have to try it out to be sure but the snippets look great to me! This is what i was hoping for when creating the original issue

@Gabriella439
Copy link
Collaborator

@basile-henry: It looks like :load was broken even before your change. Here is an example of how to trigger the same problem without multi-line input:

$ dhall repl
Welcome to the Dhall v1.32.0 REPL! Type :help for more information.
⊢ :let a = "1\n2\n"

a : Text

⊢ a

''
1
2
''

⊢ :save
Context saved to `.dhall-repl-0`

⊢ 
Goodbye.

$ cat .dhall-repl-0 
:let a = ''
1
2
''

@Gabriella439
Copy link
Collaborator

To fix some of the build failures, I think you will need to:

  • Update the stack.yaml file to use repline-0.4
  • Pass the -f-terminfo flag to cabal2nix when generating haskeline.nix

@basile-henry basile-henry marked this pull request as ready for review June 21, 2020 08:46
@basile-henry
Copy link
Collaborator Author

repline-0.4 has been released on Hackage! 😄
I have updated the nix setup and the stack setup to reflect this, and this PR should be complete!

I do have an issue with the stack setup. For some reason hnix that we use in dhall-nix doesn't like haskeline-0.8:

➜ stack build

Error: While constructing the build plan, the following exceptions were
encountered:

In the dependencies for hnix-0.7.1:
    haskeline-0.8.0.0 from stack configuration does not
                      match >=0.7.4.2 && <0.8  (latest matching version
                      is 0.7.5.0)
needed due to dhall-nix-1.1.15 -> hnix-0.7.1

Some different approaches to resolving this:

  * Set 'allow-newer: true'
    in /home/basile/.stack/config.yaml to ignore all version constraints and build anyway.

  * Recommended action: try adding the following to your extra-deps
    in /home/basile/dev/dhall-lang/dhall-haskell/stack.yaml:

- haskeline-0.7.5.0@sha256:2d7f14c0d2e4054c0628216c2ed8e7de9f9c0b6f55f7026d5fe26f7726f40e45,4833

Plan construction failed.

I don't understand why this is happening because as far as I can tell hnix-0.7.1 doesn't have any version bounds on haskeline on Hackage: https://hackage.haskell.org/package/hnix-0.7.1
I obviously can't have two different versions of haskeline in stack, so I have to use version 0.8. The 'allow-newer: true' bit seems to work but we probably don't actually want to enable that for all the packages, do we?
Is there a more fine grained option in stack? Something equivalent to nix' doJailbreak?

@Gabriella439
Copy link
Collaborator

@basile-henry: I pushed a couple of changes which will hopefully fix the build failures (or at least get further)

@basile-henry
Copy link
Collaborator Author

Great thanks! 🤞

@Gabriella439
Copy link
Collaborator

@basile-henry: Great work! Thank you for doing this 🙂

@Gabriella439 Gabriella439 merged commit 8effcf2 into master Jun 21, 2020
@Gabriella439 Gabriella439 deleted the basile/multi-line branch June 21, 2020 22:30
@sjakobi
Copy link
Collaborator

sjakobi commented Jun 23, 2020

Just tried this in the new dhall-1.33.1. Really cool! :)

It even works with :let!

⊢ :paste
-- Entering multi-line mode. Press <Ctrl-D> to finish.
| :let x =
| 1 + 2
| 

x : Natural

Thanks @basile-henry and everyone else involved! :)

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

Successfully merging this pull request may close these issues.

Multiline ability in repl
4 participants