kiyoon / jupynium.nvim

Selenium-automated Jupyter Notebook that is synchronised with NeoVim in real-time.
MIT License
486 stars 15 forks source link

add option to allow user to configure floating window #71

Closed matthewsia98 closed 1 year ago

matthewsia98 commented 1 year ago

Allow users to configure floating win for :JupyniumKernelHover by passing win_options key in setup

require("jupynium").setup({
    win_options = {
        border = "rounded",
    },
})
kiyoon commented 1 year ago

Thanks for the PR! However, this defeats one principle of this plugin: all settings must be Jupynium's vim plugin version agnostic.
Since this plugin should interact with multiple instances of neovim, sometimes even remote ones over SSH, this plugin operates a little bit in a special way: it should run even without installing the vim plugin (and only using the python package pip install jupynium).

The benefits of this approach are:

There are four things that should be modified from here.

  1. Use pcall(require( ... )) to bypass errors when the jupynium module is not found.
  2. I recently made changes to the lines in the PR #70. You should rebase it and change lines to markdown_lines
  3. Add the default option to the lua/jupynium/options.lua's M.default_opts as well. You can leave the default option in the helpers.lua file in case the plugin is not installed and the pcall require fails.
  4. Update README about the default configuration. Add some comments about what it does (i.e. it is used for :JupyniumKernelHover)
kiyoon commented 1 year ago

We can also make the option grouped to the kernel hover just in case in the future there are different types of floating windows we may use, and different options that may be added for hover (like timeout etc.). And name it floating_win_opts to make it more clear what it does.

require("jupynium").setup({
  kernel_hover = {
    floating_win_opts = {
      border = "rounded",
    },
  },
})
kiyoon commented 1 year ago

I'd prefer if we use kernel_hover.floating_win_opts than floating_win_opts.hover because there might be more settings for hover.

Also, can you add max_width = 84 in the default as well?

kiyoon commented 1 year ago

I think it's ready to be merged if you have no more comments!

matthewsia98 commented 1 year ago

I think it's ready to be merged if you have no more comments!

all good from me

kiyoon commented 1 year ago

I really appreciate this PR!