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

[WIP] add a comint-based backend for the Julia REPL #51

Closed
wants to merge 1 commit into from

Conversation

dellison
Copy link
Contributor

@dellison dellison commented Jan 19, 2019

See issue #26

I'm happy to say that this turned out to be less effort than I was expecting!

My basic approach here is to try isolate the term-specific stuff from the rest of the package, and then reimplement that functionality with comint. There's a new custom setting julia-repl-buffer-backend that can either be 'term (the default) or 'comint, and the appropriate backend for the REPL buffer is used accordingly.

A couple notes:

  • The comint REPL doesn't persist its REPL state the way the term one does (e.g., after doing ]status, you're put back at the julia> prompt instead of the pkg one). At this time I'm not really sure what a good approach to changing this would be, and I'm not really bothered by it personally so I didn't spend time trying to fix it. But it doesn't match the behavior of the "real" REPL so it might be worth it.
  • I also made customizable faces for the different prompts (but left them off my default). They don't seem to work perfectly- the julia> prompt is colored when the buffer created, and the other promts are colored when you switch to them, so setting julia-repl-comint-use-prompt-faces to t mid-session means that the help/shell/pkg prompts will be colored but the regular julia> prompt won't be. Not really a serious problem, but still a bug.
  • The term one is a lot prettier to look at since it gets all the right colors from julia itself... comint does not do this. It's probably worth fixing this, either by trying to get them from julia (maybe with color=yes) or by implementing them in elisp as part of the mode.

I've marked this as a work-in-progress because I think it probably needs a little more work before it's ready to be merged (if you do decide you want to merge it, that is). I haven't really used julia-repl all that much, so I thought it would be good to get some feedback early.

I'm not at all in a big hurry to get this merged, so please feel free to take your time reviewing it. If you want to try it out, I would especially appreciate to see use cases where the term implementation does something better than the comint version so that I can ideally bring it up to feature parity (but any other feedback you have about other things or just in general would be great too, of course).

Thanks! :)

julia-repl.el Outdated
(mapc (lambda (k)
(define-key term-raw-map k (global-key-binding k)))
julia-repl-captures))
(case julia-repl-buffer-backend
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(case julia-repl-buffer-backend
  (term (do-term-stuff)
  (comint (do-comint-stuff))

...is a pattern I use in a few places. It might be worth at least adding some error checking (in case the value isn't 'comint or 'term) or even making a function or macro that wraps this, but I'm not sure.

Copy link

Choose a reason for hiding this comment

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

you could use either cl-ecase or pcase-exhaustive which would both error on unmatched cases.

(pkg-bracket-match
(comint-simple-send proc (format "using Pkg; pkg\"%s\"" (match-string 2 string))))
(shell-sc-match
(comint-simple-send proc (format "run(`%s`)" (match-string 2 string))))
Copy link
Contributor Author

@dellison dellison Jan 19, 2019

Choose a reason for hiding this comment

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

Turning ;pwd into run(`pwd`) means you'll get a Process object back, which doesn't match the "real REPL."

shell> pwd
/home/david/projects/julia-repl
Process(`pwd`, ProcessExited(0))

I don't think this is necessarily a problem (in fact I even think it's kind of nice), but thought I'd point it out anyway.

@tpapp
Copy link
Owner

tpapp commented Jan 22, 2019

Thanks for this PR and #52. I am very busy now and may not get to review this this week, though I will do my best. I appreciate your patience.

@dellison
Copy link
Contributor Author

Not a problem at all! Take all the time you need.

;; Expand and leave it visible in buffer.
(comint-replace-by-expanded-history t pmark)
(buffer-substring pmark (point))))
(jl-call (format "Base.parse_input_line(\"%s\")" input))
Copy link
Contributor Author

@dellison dellison Jan 24, 2019

Choose a reason for hiding this comment

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

A quick note to help with reviewing, since this is command has a lot of complicated-looking elisp: everything above this line with jl-call is copied exactly out of comint-send-input. In prose, this command's purpose is: "do exactly what comint-send-input would normally do, except if it's incomplete julia code at a julia> prompt. If so, just newline and indent."

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the explanation.

@tpapp
Copy link
Owner

tpapp commented Jan 24, 2019

Thank you very much for this PR. I am testing it now, and I find it is very nice, I find the string filtering a nice solution.

That said, I am worried about the code duplication becoming a burden in the long run. I am wondering if there would be a more minimal way to implement this, which I need to think about. Specifically (I am not asking you to do this, just thinking about it), would it make sense to switch a running julia process back and forth between comint and term modes? this could be advantageous, especially if one could toggle it with a keybinding, as it would allow the smooth experience of comint (no messed up terminals etc) yet switch back to terminal mode for those processes that require it. The only thing preventing this would be the fact that the terminal type is fixed (AFAIK) when Julia starts up. However, if it works, we could make the backend buffer-local for the inferior buffer.

I find commit exciting because it would also enable us to write commands to the Julia process, and optionally parse the output. This could make editor messages like #52 or package activation not echo in the prompt (potentially invisible), allow us to start up a LanguageServer process, etc.

I ask for your kind patience while I think about this. I will eventually merge the contents of this PR in some form.

@dellison
Copy link
Contributor Author

You're welcome! It may surprise you, but I've actually been really enjoying working on this :)

I am worried about the code duplication becoming a burden in the long run. I am wondering if there would be a more minimal way to implement this, which I need to think about.

Yes, I totally agree. I've been thinking of this branch as sort of a proof-of-concept or prototype implementation, just to show that it's possible to get a more "emacs-y" Julia repl buffer that still keeps a lot (potentially all, given enough effort and elisp) of the great features in Julia's built-in terminal interface. Now that it's useable, I think it makes a lot of sense to take a step back and think carefully about how this relates to the project's longer-term goals, and then probably rework things based on those ideas.

Specifically (I am not asking you to do this, just thinking about it), would it make sense to switch a running julia process back and forth between comint and term modes?

This is an interesting idea! As far as I know, the only "entrypoint" into comint is make-comint-in-buffer, which takes a program argument that could be either (from the version 26.1 docstring):

- a string, denoting an executable program to create via
  ‘start-file-process’
- a cons pair of the form (HOST . SERVICE), denoting a TCP
  connection to be opened via ‘open-network-stream’
- nil, denoting a newly-allocated pty.

So it sounds like attaching to an already-running process should be possible to do with comint, through the second option. Beyond that, I'm not really sure. I don't know as much about term.

Anyway, yeah, there's definitely a lot to think about here. Of course, in the meantime, please do feel free to let me know if there are any changes that you'd like to see, or anything else. :)

@nverno
Copy link

nverno commented Apr 21, 2019

An unsolicited idea:
You could conditionally bind the mode changing keys to only be active when immediately following the comint prompt using a menu filter, eg. conditonal bindings.

@dellison
Copy link
Contributor Author

Cool, thanks @nverno. I'll look into using a similar macro for those keybindings and see if I like it better.

@tecosaur
Copy link

tecosaur commented Sep 4, 2021

This sounds like it's rather promising, @dellison I can't see why you closed this PR?

@dellison
Copy link
Contributor Author

dellison commented Dec 9, 2021

@tecosaur Sorry for the late reply! As I remember it, I wasn't using julia-repl myself anymore and was no longer keeping the PR up to date. Now that I reread the thread I see @tpapp asking for patience and an intention to merge eventually, which I must have forgotten about when I closed this- sorry about that! I'd be glad to reopen this if there's any interest. I can't promise to work on bringing it up to date again, but who knows.

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.

None yet

4 participants