TerminalFi / LSP-copilot

GitHub Copilot support for Sublime Text LSP plugin provided through Copilot.vim.
676 stars 25 forks source link

feat: support copilot-1.39.0 #201

Closed TerminalFi closed 3 months ago

TerminalFi commented 3 months ago

TODO

VSCode references only consider the first cursor and considers all opened files.

Example

Let’s assume the three files are:

1.  file1.py
2.  file2.py
3.  file3.py

And each file has the following first cursor/selection positions:

•   file1.py: Cursor at line 10, character 5.
•   file2.py: Cursor at line 20, character 15.
•   file3.py: Cursor at line 30, character 25. Cursor at line 45, character 40

We will only reference

•   file1.py: Cursor at line 10, character 5.
•   file2.py: Cursor at line 20, character 15.
•   file3.py: Cursor at line 30, character 25.

What needs to happen

def prepare_conversation_turn_request(
    conversation_id: str,
    window_id: int,
    message: str,
    view: sublime.View,
    source: Literal["panel", "inline"] = "panel",
) -> CopilotRequestConversationTurn | None:
    if not (doc := prepare_completion_request_doc(view)):
        return None

    # References can technicaly be across multiple files
    # TODO: Support references across multiple files
    references: list[CopilotRequestConversationTurnReference | CopilotGitHubWebSearch] = []
    visible_range = st_region_to_lsp_range(view.visible_region(), view)
    for selection in view.sel():
        if selection.empty() or view.substr(selection).isspace():
            continue
        references.append({
            "type": "file",
            # included, blocked, notfound, empty
            "status": "included",
            "range": st_region_to_lsp_range(selection, view),
            "uri": filename_to_uri(file_path) if (file_path := view.file_name()) else f"buffer:{view.buffer().id()}",
            "visibleRange": visible_range,
            "selection": st_region_to_lsp_range(selection, view),
            "position": st_point_to_lsp_position(selection.begin(), view),
            # openedAt: ni.Type.Optional(ni.Type.String()),
            # activeAt: ni.Type.Optional(ni.Type.String()),
        })

This needs to be updated to follow the logic like

for view in session(window_id):
    create_reference(view)

We need to list only the views for the window_id tied to the session

In effort to continue to show the value of this and maximize the usage of MiniHTML and drive more features we should expand to allow for expandable and collapsable references. :D

jfcherng commented 3 months ago

I think we first have to update the those typedict schemas for the new server.

jfcherng commented 3 months ago

chore: bump server to 1.39.0 for testing this PR

TerminalFi commented 3 months ago

@jfcherng I think we might need to rethink how we store conversation long term. But it is almost working. However I am having issues with jinja2 templating. Mind taking a look?

The below code causes this error Parse Error at line 351 column 28: closing p does not match opening div

{%- if section.kind == "report" -%}
<div class="references">
{%- if section.references_expanded -%}
  <a class="reference_toggle" href='{{ section.toggle_references_url }}'>{{ section.references|length }} References</a>
  <div class="reference">
{% for reference in section.references %}
      <a href="#">{{ reference['uri'] }}</a>
{% endfor %}
  </div>
{% else %}
  <a class="reference_toggle" href='{{ section.toggle_references_url }}'>{{ section.references|length }} References</a>
{%- endif -%}
</div>
{%- endif -%}
jfcherng commented 3 months ago

The below code causes this error Parse Error at line 351 column 28: closing p does not match opening div

That's caused by how mdpopup handles mixed HTML+MD iirc (indented HTML is somehow recognized as MD?). I encountered that before. That <p> is inserted by mdpopup but it inserts </p> in a wrong place, where should be </div>.

jfcherng commented 3 months ago

I think we might need to rethink how we store conversation long term.

I see you inject references_expanded into the payload from Copilot. I think we should create a new type and keep the original payload type stay intact (payload only represents what we received from the server as-is). Just like why we created CopilotPayloadConversationEntryTransformed (can we reuse this?).