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

Code Runner Overhaul #99

Merged
merged 8 commits into from
Nov 18, 2023
Merged

Code Runner Overhaul #99

merged 8 commits into from
Nov 18, 2023

Conversation

benlubas
Copy link
Contributor

@benlubas benlubas commented Nov 1, 2023

closes #93
closes #81

General notes

  • I've pulled config out into it's own file
  • The api hasn't changed, ie there are still :QuartoSend and require('quarto').quartoSend() functions (for now, thinking about removing the old lua functions), but there's now also require('quarto.runner').run_cell(multi_lang: bool) functions that give the user control over if the function runs multiple languages at once. I'm completely open to changing these function names if you want them to be more analogous to the current "send", "sendCell" etc.

Things that are working

  • All of the 'old' code running functions
  • a new QuartoSendLine
  • configuring your code runner (currently the only options are Molten and Slime) with default and on a per language basis.
  • excluding some languages from being run ever (default to just ignore yaml)

More changes before this PR is ready

  • I'm definitely going to refactor the way code runners work so they each have their own file, and export a common table of functions that we can use to cleanup the send function a little.
  • README needs an update to talk about these changes.
  • config defaults should be changed so that code running doesn't work by default (b/c the user has to install another plugin for it to work)

Things I'm pushing off until a later PR

  • auto target switching. This ended up being too much for right now. I think that I want to discuss what exactly this should look like more in depth. The challenge here I think will be handling different runners doing different things and provide a good UX for this

@benlubas
Copy link
Contributor Author

benlubas commented Nov 1, 2023

@jmbuhr Curious to hear what you think about removing the require('quarto').quartoSend() and similar functions.

It is a breaking change for existing users, but so is the new way that code runners are configured.

@benlubas benlubas marked this pull request as ready for review November 2, 2023 15:20
@jmbuhr
Copy link
Collaborator

jmbuhr commented Nov 2, 2023

Looks good, I'll play around with it on the weekend! :)

@jmbuhr
Copy link
Collaborator

jmbuhr commented Nov 10, 2023

This is shaping out nicely!

@jmbuhr
Copy link
Collaborator

jmbuhr commented Nov 11, 2023

  • I believe we don't need the get_config function, we can simply use local config = require("quarto.config").config in those places.
  • Feels great to use so far, I tried with slime and molten

Questions or requests for molten that would tie in nicely with the notebook experience (not required for the integration, just nice to haves):

  • Is it possible to navigate easily into the output window, like with K where the first press opens the hover and the second moves the cursor there for easy copy-pasting?
  • Is it possible to keep multiple output floats open at the same time to create a jupyter notebook style experience? e.g. one might want to keep the chunk output of a dataframe visible while working on a plot to remember the column names or work on multiple plots. With slime I would do so using web graphics devices (e.g. httpgd for R) that show the plots in the browser via a server, but molten could potentially deliver this out of the box.

@benlubas
Copy link
Contributor Author

I was at some point having issues without the get_config function. I'll test without it.

Is it possible to navigate easily into the output window

Yup, third keybinding here

keep multiple output floats open at the same time

Currently no, and also going to take some time b/c the current code assumes everywhere that there is only one output open at a time and that output is on screen and below the top line. But that is something I'm working on (here is the issue). I might implement other things before this gets finished but it'll get there eventually.

@benlubas
Copy link
Contributor Author

I'm not sure what I was doing before, but it seems to work now

@benlubas
Copy link
Contributor Author

@jmbuhr lmk if there's anything else that you want me to do with this.

Also letting you know that I've added virtual text output to molten, which allows for more than one output to be open at a time. It's still 'pre-release' but it's been merged to main, and works well enough to use.

@jmbuhr
Copy link
Collaborator

jmbuhr commented Nov 18, 2023

Virtual text ist a cool solution for this! Is this ready to merge from your side?

@benlubas
Copy link
Contributor Author

benlubas commented Nov 18, 2023

@jmbuhr Yup! ready to go. I don't have anything else to add to it. I've used this for like two weeks and it seems to work pretty well.

@jmbuhr
Copy link
Collaborator

jmbuhr commented Nov 18, 2023

Perfect! I'll merge this as is then :)
Afterwards, some things might be moved around or renamed in a slight cleanup I have been meaning to do for a while.

@jmbuhr jmbuhr merged commit eacd8ff into quarto-dev:main Nov 18, 2023
@benlubas benlubas deleted the code_runner branch November 18, 2023 18:10
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.

Official Molten support? Document code sending features and vim slime dependency
2 participants