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

Add Julia layer separate from ESS #10444

Merged
merged 3 commits into from
Aug 11, 2018
Merged

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Mar 7, 2018

This PR builds a layer for Julia that is separate from ESS using julia-repl and
LSP. This is necessary because the REPL included with ESS doesn't allow proper
usage of the Julia debugger Gallium.jl. I've got a bit of cleaning up to do
here, but I was hoping somebody with more knowledge could help me out with the
below problems:

  • spacemacs-jump-handlers-julia-mode isn't getting propagated to
    spacemacs-jump-handlers, so spacemacs/jump-to-definition doesn't work
    in julia-mode buffers. (FIXED by not starting REPL automatically)
  • lsp-ui-doc from lsp-ui splashes text all across the screen instead of in a
    nice controlled buffer as shown in the lsp-ui docs. (This is caused by
    lsp-ui-doc behaves weird emacs-lsp/lsp-ui#87 . Proposed solution is to
    disable lsp-ui-doc until emacs 26 becomes stable.
    Probably just a local
    problem from weirdly configured fonts...)
  • lsp-ui-sideline from lsp-ui isn't showing any information.
  • When a file is opened with , f f, the buffer with the file contents is
    never focused. You have to manually switch to the new buffer. (FIXED by
    not starting REPL automatically)
  • Some people will still want to use ESS for Julia. How do I make this new
    layer play nice for people who want to use ESS for R but this layer for
    Julia? Do I need to set the ESS stuff up as a backend similar to the
    Python layer?
  • Fix test that I have no idea how I broke.

lsp-ui isn't necessary for this layer to be useful, so those could just be
turned off for now. The others probably need to be fixed before merging.

@non-Jedi non-Jedi force-pushed the julia_layer branch 2 times, most recently from 1f44c7b to 31517ea Compare March 8, 2018 15:10
@gdkrmr
Copy link
Contributor

gdkrmr commented Mar 9, 2018

when the ess layer is activated, files still load in ESS[julia] major mode

@gdkrmr
Copy link
Contributor

gdkrmr commented Mar 10, 2018

I just asked how to disable julia in ESS: emacs-ess/ESS#501

@non-Jedi non-Jedi force-pushed the julia_layer branch 2 times, most recently from 8943b74 to dde0cfd Compare March 15, 2018 02:10
@non-Jedi non-Jedi changed the title Add Julia layer separate from ESS [WIP] Add Julia layer separate from ESS Mar 15, 2018
@@ -36,7 +36,6 @@
("\\.do\\'" . STA-mode)
("\\.ado\\'" . STA-mode)
("\\.[Ss][Aa][Ss]\\'" . SAS-mode)
("\\.jl\\'" . ess-julia-mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

now you are simply disabling julia for ess, so no one will be able to use ess with julia. Not sure what would be the best solution, but probably a flag like
(defvar ess-use-julia t)
and then disable stuff conditionally
then I can simply specify in my .spacemacs

(setq-default
 dotspacemacs-configuration-layers
 (ess :variables
       ess-use-julia nil))

Copy link
Contributor Author

@non-Jedi non-Jedi Mar 23, 2018

Choose a reason for hiding this comment

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

When I get back to this, the plan is to move all of the julia-related ESS stuff into the julia layer. This change was why I marked this PR as WIP since using my branch could very easily break workflows until I finish that migration. So .jl files will still open in ess-julia-mode (for those who customize this layer to do so), but you'll have to install the julia layer for that rather than the ESS layer. Basically just what you said above, but the flag would be on the julia-layer side rather than the ESS side.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure, where you are going with this. Please don't break my ESS julia setup. ESS has quite decent Julia support out of the box for now (which does not mean it cannot be improved) and this should be unchanged if only the ess layer is installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want it to be, your experience will be identical; you'll just have to add the julia layer in addition to the ESS layer. If ESS provides functionality that's only applicable to julia (it does), then it makes no sense to keep it in the generic ESS layer instead of moving it to a layer specifically for julia.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing with ESS (as in the emacs package to use R/STATA/Julia/S/..., in contrast to ess, the layer for spacemacs) is its age, on the one hand it has a lot of features and on the other it has a lot of ancient code no one has touched for ages. The current maintainers are all R users (this is my personal impression), so there is still quite a bit of ongoing development for R. Julia support was somewhat put on top of that, integration is far from perfect and would probably require a bit of work and dealing with the ESS codebase. So I am not sure if it would be better to start fresh, or to stick to ESS.

By your logic there should be an r layer instead of an ess layer, so I think that the ess layer should keep its ability to be used with julia out of the box.

Questions:

  1. Which parts of ess are julia only?
  2. you are going to use julia-repl, how is this going to give an identical experience?

@gdkrmr
Copy link
Contributor

gdkrmr commented Apr 7, 2018

I finally had time to play around with this a little bit and it looks really good.

  • lsp-mode is really nice.
  • the repl is much better than the ess one.
    the only thing that I don't like is that I had to remove the ess layer and I haven't had time to see if all the keys I use in ess are available.

I would propose to make the ess-layer in such a way that julia support can be disabled there (ess-julia-support nil).

EDIT: lsp-mode even works in terminal, but it gets messy, when both terminal and gui frames are open :-)

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Apr 7, 2018

@gdkrmr You should be able to toggle whether ess-julia-mode and the ess repl are used or vanilla julia-mode and julia-repl are used by setting julia-mode-enable-ess without uninstalling the ess layer. Is this not working for you? I still need to update the readme to document this behavior.

On lsp-mode in the REPL, that's interesting; I haven't even tried it there. What steps did you take to get it working? Just invoking lsp-julia-enable in the REPL buffer?

@gdkrmr
Copy link
Contributor

gdkrmr commented Apr 7, 2018

@gdkrmr You should be able to toggle whether ess-julia-mode and the ess repl are used or vanilla julia-mode and julia-repl are used by setting julia-mode-enable-ess without uninstalling the ess layer. Is this not working for you? I still need to update the readme to document this behavior.

That sounds good, I was not aware of this, I was trying to conditionally remove all the julia specific stuff from the ess/package.el. Does this work even though .jl files a set to ess-julia-mode in auto-mode-alist?

On lsp-mode in the REPL, that's interesting; I haven't even tried it there. What steps did you take to get it working? Just invoking lsp-julia-enable in the REPL buffer?

What I did was copying your files to the private layers directory, removing ess from the layers list and adding this to the layers list:

     (julia :variables
            julia-mode-enable-lsp t
            julia-mode-enable-ess nil)

If I run emacs -nw or over ssh it simply works, no manual enabling necessary. What did not work when I started emacs and then ran emacsclient -nw -c, that just put me in some lsp specific buffer in the terminal frame.

@gdkrmr
Copy link
Contributor

gdkrmr commented Apr 8, 2018

Setting (julia-mode-enable-ess t) opens .jl files in fundamental mode.

I added some keybindings to the ess layer some time ago and there was some discussion about spacemacs conventions, you may want to take a look at that (#9787).

I personally have the following in my muscle memory:

  • ,ss switch to repl/switch back to file
  • ,, smart eval, evals a paragraph/function, never worked well in ess-julia-mode

@x-ji
Copy link
Contributor

x-ji commented May 26, 2018

Great work. Are you still planning to update it further. Is it OK to directly try to merge your changes into my Spacemacs repo and see if it works? I don't want to have to use Atom (Juno) whenever I work on Julia projects...

@non-Jedi
Copy link
Contributor Author

@x-ji yep. Still some work to do to get this to the point where I'm happy with it. Should work fine to just merge this branch though if you're already following develop.

@non-Jedi non-Jedi changed the title [WIP] Add Julia layer separate from ESS Add Julia layer separate from ESS Jun 29, 2018
@non-Jedi
Copy link
Contributor Author

I'm happy with this PR now. Let me know how it's looking in general, and I'll squash commits and rebase on current develop.

@gdkrmr
Copy link
Contributor

gdkrmr commented Jun 30, 2018

I am using it for everyday julia programming and really like it, great work!

  • I would like to use ,sd to send a line to the repl.
  • There should be a possibility to easily disable the help window, in small windows and on functions with long doc strings it covers the entire window.

@gdkrmr
Copy link
Contributor

gdkrmr commented Jul 1, 2018

Here is another improvement, but I am not sure where this best belongs, julia-mode, julia-repl, or here:
When editing a package with julia 0.7, I open the path/to/package/src/Package.jl and then run the julia repl with ,sl. The julia repl opens in an extra buffer in the path/to/package/src/ directory, then I have to switch to the julia repl and do cd("..") and ]activate . to actually work with the package. It would be much nicer if the repl buffer could be opened directly in the package root directory.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jul 2, 2018

To be honest, I haven't even tried 0.7 yet. @tpapp, any idea what's going on above? Is that a bug with julia-repl for 0.7? Or just new hooks for integrating into the Pkg system that julia-repl doesn't take advantage of (yet)?

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jul 2, 2018

@gdkrmr What's the mnemonic supposed to be for ,sd? Function to disable help window would be lsp-ui-doc-mode I believe. If you wouldn't mind, it might be worth filing an issue against lsp-ui for the window being too large?

@tpapp
Copy link

tpapp commented Jul 2, 2018

@non-Jedi: thanks for the heads up, opened an issue, will fix it soon

@gdkrmr
Copy link
Contributor

gdkrmr commented Jul 2, 2018

@non-Jedi I am used to it from the ess layer ;-), believe me by now it is hard wired into my muscle memory, I you don't want it, I it's fine.
@tpapp great, thanks!

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jul 6, 2018

FYI, the keybindings I setup weren't actually working in julia-repl before, but they should be now.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Jul 6, 2018

@syl20bnr this has been sitting for a while because it was WIP for a while, but it's ready now. I don't really know what the structure is around here; is there someone I should tag to get this reviewed so we can start moving it towards a merge?

@gdkrmr
Copy link
Contributor

gdkrmr commented Jul 26, 2018

here is a feature you could add, so sb will surround the currently marked block with begin ... end, sq with quote ... end and s: with :(...), this should probably be made conditional on the presence of evil-surround:

  (add-hook 'julia-mode-hook
    (lambda ()
      (push '(?b . ("begin" . "end")) evil-surround-pairs-alist)
      (push '(?q . ("quote" . "end")) evil-surround-pairs-alist)
      (push `(?l . ("let" . "end"))   evil-surround-pairs-alist)
      (push '(?: . (":(" . ")"))      evil-surround-pairs-alist)))

EDIT: added let block.

@sdwolfz
Copy link
Collaborator

sdwolfz commented Aug 9, 2018

I'll go ahead and merge this layer once it's rebased with latest develop and all commits are squashed into one.

@gdkrmr
Copy link
Contributor

gdkrmr commented Aug 10, 2018

please add my suggestions before merging, I want them out of my .spacemacs!!

@sdwolfz
Copy link
Collaborator

sdwolfz commented Aug 10, 2018

@gdkrmr 👍 but if they are not added here I will accept a PR with them afterwards. They seem straightforward and will be merged fast.

@gdkrmr
Copy link
Contributor

gdkrmr commented Aug 10, 2018

@non-Jedi

@non-Jedi
Copy link
Contributor Author

@sdwolfz Thanks! I've squashed the commits down into three logical commits; let me know if you'd prefer just one. Otherwise things should be good.

@gdkrmr I'm not entirely sure how to make your addition dependent on presence of evil-surround, so I'll let you PR the difference after merge (or PR directly against this branch before merge if you'd prefer).

@sdwolfz sdwolfz merged commit 42f9ad4 into syl20bnr:develop Aug 11, 2018
@sdwolfz
Copy link
Collaborator

sdwolfz commented Aug 11, 2018

Thank you ❤️!
And congratulations on your first Spacemacs PR 🎉!
Cherry-picked into develop branch, you can safely delete your branch.

@gdkrmr
Copy link
Contributor

gdkrmr commented Aug 11, 2018

great, thanks for merging this!!!
@non-Jedi I'll figure it out.

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

5 participants