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 core.execute #618

Closed
wants to merge 15 commits into from
Closed

add core.execute #618

wants to merge 15 commits into from

Conversation

tamton-aquib
Copy link
Contributor

@tamton-aquib tamton-aquib commented Oct 9, 2022

This module is aimed to provide an execute command for neorg.

2022-10-28_23-26-32.mp4

Possible todos:

  • cross platform support.
  • spinner for running block.
  • run for
    • compiled (c, cpp, rust, etc)
      • check extra parameters for wrapping in main function? (+ named blocks)
    • interpreted (python, lua, bash, etc)
  • subcommands
    • view (virtual_lines)
    • normal (maybe a better command name, normal lines added to buffer.)
  • add logging?
  • check for timeout / limit number of output lines.
  • make non-blockable if possible (weird python behaviour)
  • user config options.
  • set lines to separate files and run.
  • panic messages instead of empty returns.
  • assign state for each running block instead of tracking the current one. (will fix spinners and re-execution bugs)
  • code cleanup.

@max397574
Copy link
Contributor

perhaps call the subcommand which is now called normal either text or insert

vim.api.nvim_buf_set_lines(
curr_task.buf,
curr_task.code_block['end'].row + 1,
curr_task.code_block['end'].row + #curr_task.output + 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be the same as the start
if not lines below the code block are overwritten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for deleting the previous output lines if present, havent experienced any bugs yet on my side(on this line).

0, 0, {}
)
-- IMP: check for existng marks and return if it exists.
local cr, _ = unpack(vim.api.nvim_win_get_cursor(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't you just get rid of the , _ ?
the value would just be ignored
also
I think cr isn't a really descriptive variable name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't you just get rid of the , _ ?

Was indexing the first item(row), but was testing with both.

I think cr isn't a really descriptive variable name

let me know what else you want to call it

end

module.config.public = require("neorg.modules.core.execute.config")
module.config.private = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to add this if it's unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was some settings on my local repo, will delete the line if its not used 👍🏻

local spinner = require("neorg.modules.core.execute.spinner")

local module = neorg.modules.create("core.execute")
local ts = require("nvim-treesitter.ts_utils")
Copy link
Contributor

Choose a reason for hiding this comment

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

we got a module for this
not sure what's preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will discuss and resolve with vhyrro 👍🏻

Copy link
Member

Choose a reason for hiding this comment

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

Here you probably want to use module.required["core.integrations.treesitter"].get_ts_utils()

@@ -0,0 +1,41 @@
-- TODO: code cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

@vhyrro perhaps add some standard location of things like this? (basically util files)

@tamton-aquib tamton-aquib changed the title add execute module base add core.execute Dec 16, 2022
@tamton-aquib tamton-aquib marked this pull request as ready for review January 16, 2023 15:40
@vhyrro
Copy link
Member

vhyrro commented Feb 23, 2023

By the way, this module is not forgotten. I think about this every now and again but just have other priorities. The thought of implementing code execution does excite me though heh

@laher
Copy link
Contributor

laher commented May 5, 2023

Hi @tamton-aquib ... I just copy-pasted this as an external module on my filesystem, and it's working beautifully for me. Very cool work. I just had to s/core.execute/external.execute/ in a few places. It's so good for my journal template to have some daily tasks which I can trigger like this - thank you 🥇

What do you think about re-creating this as an external module & publishing in its own repo, while this core module is under review? It might take a while for this to get merged as a core module, and in the meantime we could collaborate. I & others could fork & contribute. (Or I can publish my copy-paste and credit you, but that would feel like the wrong way round).

I know it belongs in core eventually, but I think that if folks could start using it now, then they would & people would help to improve it.

Thanks again

@tamton-aquib
Copy link
Contributor Author

Heyyo @laher ,

What do you think about re-creating this as an external module & publishing in its own repo, while this core module is under review? It might take a while for this to get merged as a core module, and in the meantime we could collaborate

I talked to vhyrro on discord and it might take some time for him to review this PR.
So for the time being i guess you could create the fork because i wont probably be able to maintain it 😬 .

(Or I can publish my copy-paste and credit you, but that would feel like the wrong way round)

This would be better. I guess you could just link this PR instead of credits btw 😅

PS: i dont know if the code is ugly, so feel free to refactor or anything hehe

laher added a commit to laher/neorg-exec that referenced this pull request May 5, 2023
@laher
Copy link
Contributor

laher commented May 5, 2023

Cool, I'll do that. I haven't written much lua myself, but what you've done is working & simple enough that I could add a couple of languages easily.

I'll add some linting and stuff and go from there...

I'll pop onto discord for discussion once I have something.

@laher
Copy link
Contributor

laher commented May 6, 2023

Hi again, I've been reading the specs and getting my head around tags and norg generally, and I think I know where I'd like to take this.

Repo: https://github.com/laher/neorg-exec

  • I named it external.exec because it's 3 whole characters shorter, and I'm lazy. I gave it a GPL3 like neorg because it matches.
  • I think it'd be tidy to use ranged tags for the results sections, so it's easier to address them for re-runs. (could be verbatim or standard, depending on the use case).
  • I'd like to use tags to indicate how to run the code block, something like #exec cache=5m pwd=.. result.tagtype=|
  • also tags to present metadata about the results. exit code, code-hash for cached results, timings, etc.
  • I'd like to use popups instead of virtual lines for the virtual mode.

See the planning section for examples of what this might look like: https://github.com/laher/neorg-exec#planning

I'm very new to norg, so I'd appreciate some feedback about how well this tagging approach might work with norg.

OK I'll stop spamming this PR now and move discussion into discord + my repo.
Ta

@katawful katawful mentioned this pull request May 18, 2023
1 task
@tamton-aquib
Copy link
Contributor Author

Closing the PR as the development regarding this features is moved to an external module:
https://github.com/laher/neorg-exec 🎉

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.

4 participants