zbirenbaum / copilot-cmp

Lua plugin to turn github copilot into a cmp source
MIT License
1.13k stars 41 forks source link

Formatting fixes #67

Closed zbirenbaum closed 1 year ago

zbirenbaum commented 1 year ago

This is a long overdue cleanup/overhaul of how formatting of response text from copilot is handled since most of the defensive measures implemented are no longer necessary and break things instead. Anecdotally it seems to have far less issues especially regarding closing punctuation such as } or )

Since this removes a ton of config features that I'm not really sure anyone is using I am going to keep this as a PR until there is some user feedback instead of just merging.

I do think there are probably some remaining improvements to be made by utilizing cmp's filter and execution functionality, but for the moment this should be a fairly sizable improvement for most users if my experience is anything to go by.

hthuong09 commented 1 year ago

There is a difference between the master branch and this. I'm not sure if I did something wrong.

I have this code

const response = await request(app.getHttpServer())
  .get(`<cursor is here>`)
  .set(headers)
  .expect(200);

When I type path, then press enter The copilot auto-completion will be

.get(`path/to/${param}

With this branch

const response = await request(app.getHttpServer())
  .get(`path/to/${param} // <-- missing `)
  .set(headers)
  .expect(200);

With master branch

const response = await request(app.getHttpServer())
  .get(`path/to/${param}`)
  .set(headers)
  .expect(200);

Both cmp mapping do not help

mapping = {
    ["<CR>"] = cmp.mapping.confirm({ behavior = cmp.ConfirmBehavior.Replace, select = true }),
},

---

mapping = {
    ["<CR>"] = cmp.mapping.confirm({ behavior = cmp.ConfirmBehavior.Insert, select = true }),
},

How do I keep the behavior on master branch?

zbirenbaum commented 1 year ago

There is a difference between the master branch and this. I'm not sure if I did something wrong.

Ok so I know why this is happening, but I'm not sure what the best way to fix it without falling into the same traps that led to a ton of bloat that this PR cleans up.

Essentially what is going on here is that copilot is seeing that ) at the end of the line and trying to give you a completion without it.

If it was consistent about this, that's an easy fix, but it seems sometimes (usually in my experience) copilot will give a response with the closing parenthesis as well.

I think it is re-fetching completions after typing the open parenthesis, and then displaying them as a better match, since cmp is probably filtering out the closing parenthesis completion (the one we want), since it should not immediately follow the current character in the completion string.

zbirenbaum commented 1 year ago

There is a difference between the master branch and this. I'm not sure if I did something wrong.

Ok, I pushed some changes that fixed this behavior in every instance I tried it in. I'm pretty sure this is by far the best copilot-cmp has ever functioned. According to some issues and a medium article I found while researching the topic I'm pretty sure it is now handled better than VSCode by a substantial margin. In my tests this felt smooth and I would really like to merge it if your issue is solved and everything looks good.

hthuong09 commented 1 year ago

@zbirenbaum thanks for the quick fix! I'll try it out and let you know if all is good.


Edit: The completion is now

.get(`path/to/${param})

Which will be completed as

const response = await request(app.getHttpServer())
  .get(`path/to/${param}) <-- wrong syntax
  .set(headers)
  .expect(200);

If I change ` to ' it work correctly though

const response = await request(app.getHttpServer())
  .get('path/to/' + param)
  .set(headers)
  .expect(200);

Edit 2: I think it's a very edge case that I'm testing. If I'm typing normally from .get to get completion it works correctly. Let me use it for a bit longer to see if there are any other concerns.

zbirenbaum commented 1 year ago

A couple very minor things with punctuation still seem to be an inconsistent problem, but on the whole it's a vast improvement and I haven't seen any major complaints so I'm going to to go ahead and merge this.