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 option to share Magma between buffers #106

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phcerdan
Copy link

@phcerdan phcerdan commented Sep 5, 2023

Initialize magma as usual: :MagmaInit or :MagmaInit kernel_name
Change buffer in neovim and use shared option:

:MagmaInit kernel_name shared

TODO: Improve argument parsing for :MagmaInit

Now, the same kernel is shared between two buffers

Fix #105

Initialize magma as usual: `:MagmaInit` or `:MagmaInit kernel_name`
Change buffer in neovim and use shared option:

`:MagmaInit kernel_name shared`

TODO: Improve argument parsing for :MagmaInit

Now, the same kernel is shared between two buffers
@phcerdan phcerdan marked this pull request as draft September 5, 2023 10:20
@phcerdan
Copy link
Author

phcerdan commented Sep 5, 2023

This is POC for #105
It works, but requires:
TODO:

  • Improve argument parsing for MagmaInit
  • Format (black?)

@benlubas
Copy link

benlubas commented Oct 4, 2023

Improve argument parsing for MagmaInit

@phcerdan What do you mean by this? I'd love to have this feature as well, so I'm happy to put some work in to get it over the finish line. Although it will probably end up just living in my fork instead of getting into this upstream at the moment

@phcerdan
Copy link
Author

phcerdan commented Oct 5, 2023

Probably your fork will become the main, @WhiteBlackGoose doesn't seem to have to the time?

This PR is working, but I requires some love on how to pass the shared option to the :MagmaInit command.

MagmaInit right now allows 0 or 1 arguments. This PR introduces that if there are at least 2 arguments, :MagmaInit workenv foo boo this_is_shared the existing kernel (workenv) is shared in the current buffer.

    @pynvim.command("MagmaInit", nargs="*", sync=True, complete='file')  # type: ignore
    @nvimui  # type: ignore
    def command_init(self, args: List[str]) -> None:
        self._initialize_if_necessary()

        if args:
            args = args[0].split(" ")
            kernel_name = args[0]
            shared = False
            if len(args) > 1:
                shared = True
            print(args)

That argument logic has to be improved. That is the only missing part from WIP to ready IMO.

@benlubas
Copy link

benlubas commented Oct 6, 2023

Unfortunately this PR doesn't handle io.py,

It's like a 4 line change if we don't expand the functionality to allow loading into the same kernel.
I haven't spent much time looking into loading into a shared kernel. I consider that a nice to have, but something that's not necessary.

👀 it looks like it might be really easy to do

I regret saying that

@benlubas
Copy link

benlubas commented Oct 7, 2023

Preliminary PR is up: benlubas/molten-nvim#4

I got save and load to a point where it seems like they would work, but I must have broken save/load at some point along the way b/c it just doesn't work in my version of the plugin at all (i have saving to a file, and reading from a file working without error, but trying to open a cell after it's been loaded produces output that says it's loading, and it just stays like that), and just confirmed that's how magma behaves as well

None-the-less, @phcerdan, if you're able to test that branch that'd be awesome. Don't feel obligated, I'm going to be using it as well, so hopefully it can be merged.

Update, it's merged, and Molten is now stable

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.

[Feature Request] Add global option to MagmaInit. Share the same kernel for all buffers.
2 participants