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

rewrite and append with full file context #142

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

Conversation

tommyengstrom
Copy link

This adds support for including the whole file in addition to the selected text in rewrite and append selection.

I don't expect this to be merged as is. Mostly wanted to ask if you'd like to add this if I clean it up a bit. Let me know what you think.

@quolpr
Copy link

quolpr commented Jun 23, 2024

@Robitx this looks awesome, could you merge?

lua/gp/init.lua Outdated
@@ -2494,6 +2508,7 @@ end

M.Prompt = function(params, target, prompt, model, template, system_template, whisper)
-- enew, new, vnew, tabnew should be resolved into table
print(template)
Copy link

Choose a reason for hiding this comment

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

Looks like a debug print

lua/gp/init.lua Show resolved Hide resolved
lua/gp/init.lua Outdated
@@ -603,6 +603,15 @@ _H.find_git_root = function()
return ""
end

_H.repo_instruct_file = function()
Copy link

Choose a reason for hiding this comment

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

Is it needed?

@@ -158,12 +130,26 @@ local config = {
-- templates
template_selection = "I have the following from {{filename}}:"
.. "\n\n```{{filetype}}\n{{selection}}\n```\n\n{{command}}",
template_rewrite = "I have the following from {{filename}}:"
template_rewrite = "I have the following awesome stuff from {{filename}}:"
Copy link

Choose a reason for hiding this comment

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

Do we need "awesome"?

@@ -36,11 +36,11 @@ local config = {
-- agents = { { name = "ChatGPT4" }, ... },
agents = {
{
name = "ChatGPT4",
name = "ChatGPT4o",
Copy link

Choose a reason for hiding this comment

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

Should we really switch to gpt-4o? I think it should be different PR

@tommyengstrom
Copy link
Author

hehe. yeah, this is not intended as a final PR, just checking if there's interest.

Should I go ahead an clean it up a bit? I.e. mostly doing what @quolpr suggested.

@1orZero
Copy link
Contributor

1orZero commented Jun 25, 2024

Definitely interested! Thank you for creating this PR!

Looks like there are conflicts to be resolved.

@gonzaloserrano
Copy link

+1 interested in this, and @quolpr suggestions such as extracting the 4o model change to a new PR.

@tommyengstrom
Copy link
Author

@Robitx Any thoughts on this?

@Robitx
Copy link
Owner

Robitx commented Jul 16, 2024

@tommyengstrom Hey, sorry for not looking into it sooner.

Yes this is heading in a good direction, but I would like to prevent combinatorial explosion for commands based on the context, there are many possible contexts people might want to use:

  • no context
  • .gp.md repo instructions
  • current file
  • all visible buffers
  • all open buffers
  • context build from LSP/Treesitter analysis
  • ...

So I would go in the direction of a separate set of commands similar to https://github.com/Robitx/gp.nvim?tab=readme-ov-file#agent-commands for switching the type of context which will be used by the commands such as GpRewrite.

@tommyengstrom
Copy link
Author

I suspect most of those examples are not really going to be used, thus making the "combinatorial explosion of commands" a non issue. I considered adding LSP output in this command but haven't done so. If done I don't see a point in having a separate command without it.

Either way, I'm not interested in spending time on implementing that.

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