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

Minor restructure for pynvim 0.4.3 backwards compatibility #34

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

errnoh
Copy link
Contributor

@errnoh errnoh commented Feb 9, 2024

I was wondering why just cloning the repo to rplugin/python3 and running :UpdateRemotePlugins didn't work and spent some time looking into it. Turns out that it should, but some issues in the structure prevent that. To fix that I modified the rplugin and related imports to work similarly to the example pynvim module.

pros:

  • No longer requires manual steps, just clone it to the correct location as expected.
  • Fixes things like Cannot install due to python issues #16 (relative imports now correctly identify that it's a package)
  • Easier to add the package to nixpkgs etc

cons:

  • All local imports done with * imports, making it a bit harder to know which file the functions are coming from. No idea why from .XXX import * works but from .XXX import Y, Z doesn't.

@jellydn jellydn added the enhancement New feature or request label Feb 9, 2024
@jellydn
Copy link
Contributor

jellydn commented Feb 9, 2024

Thanks @errnoh for your work.

why just cloning the repo to rplugin/python3 and running :UpdateRemotePlugins didn't work

I tried the manual install on the 1st version from @gptlang and it works for me.

Turns out that it should, but some issues in the structure prevent that

Could you elaborate on this point? What is your neovim setup, python version?

@errnoh
Copy link
Contributor Author

errnoh commented Feb 9, 2024

@jellydn hmm, interestingly enough I think the main issue seems to be that I had two pynvim versions installed and CopilotChat used one for it's heatlhcheck and vim actually used the other one for executing commands.

What this PR actually seems to do is at least provide backwards compatibility with pynvim 0.4.3. (modified PR title to mirror this)


With the repo cloned to ~/.config/nvim/pack/vendor/start, no additional commands other than require('CopilotChat').setup{}:

main branch

- Python version: 3.11.6
- pynvim version: 0.4.3
> :UpdateRemotePlugins
Encountered ModuleNotFoundError loading plugin at /home/errnoh/.con
fig/nvim/pack/vendor/start/CopilotChat.nvim/rplugin/python3/copilot
-plugin.py: No module named 'handlers'                             
Traceback (most recent call last):                                 
  File "/nix/store/a5y5ik9hwz1n8jf5y6a8hfwmd5r84w9l-python3-3.11.6-
env/lib/python3.11/site-packages/pynvim/plugin/host.py", line 165, 
in _load                                                           
    module = imp.load_module(name, file, pathname, descr)          
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^          
  File "/nix/store/rac8pxbi1vapwrlqzbrkycbyg521djzw-python3-3.11.6/
lib/python3.11/imp.py", line 235, in load_module                   
    return load_source(name, filename, file)                       
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                       
  File "/nix/store/rac8pxbi1vapwrlqzbrkycbyg521djzw-python3-3.11.6/
lib/python3.11/imp.py", line 172, in load_source                   
    module = _load(spec)                                           
             ^^^^^^^^^^^                                           
  File "<frozen importlib._bootstrap>", line 721, in _load         
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
                                                                   
ModuleNotFoundError: No module named 'handlers'                    
                                                                   
Encountered ModuleNotFoundError loading plugin at /home/errnoh/.con
fig/nvim/pack/vendor/start/CopilotChat.nvim/rplugin/python3/copilot
.py: No module named 'prompts'                                     
Traceback (most recent call last):                                 
  File "/nix/store/a5y5ik9hwz1n8jf5y6a8hfwmd5r84w9l-python3-3.11.6-
env/lib/python3.11/site-packages/pynvim/plugin/host.py", line 165, 
in _load                                                           
    module = imp.load_module(name, file, pathname, descr)          
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^          
  File "/nix/store/rac8pxbi1vapwrlqzbrkycbyg521djzw-python3-3.11.6/
lib/python3.11/imp.py", line 235, in load_module                   
    return load_source(name, filename, file)                       
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                       
  File "/nix/store/rac8pxbi1vapwrlqzbrkycbyg521djzw-python3-3.11.6/
lib/python3.11/imp.py", line 172, in load_source                   
    module = _load(spec)                                           
             ^^^^^^^^^^^                                           
  File "<frozen importlib._bootstrap>", line 721, in _load         
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
                                                                   
ModuleNotFoundError: No module named 'prompts'                     
                                                                   
remote/host: python3 host registered plugins ['mymodule']          
remote/host: generated rplugin manifest: /home/errnoh/.local/share/
nvim/rplugin.vim                                

(mymodule is not related)

errnoh/dev branch

- Python version: 3.11.6
- pynvim version: 0.4.3

Copilotchat.nvim health check:
- WARNING python 3 is required
> :UpdateRemotePlugins
remote/host: python3 host registered plugins ['CopilotChat', 'mymodule']                              
remote/host: generated rplugin manifest: /home/errnoh/.local/share/nvim/rplugin.vim
:CopilotChatExplain

### User
Explain how it works.

### Copilot

<expected response here>

With the 0.4.3 removed and only using 0.5.0:

- Python version: 3.11.6
- pynvim version: 0.5.0

Copilotchat.nvim health check:
- OK Python version 3.11 is supported
- OK pynvim version 0.5.0 is supported

Both main and errnoh/dev work fine.

@errnoh errnoh changed the title Minor restructure for more idiomatic installation Minor restructure for pynvim 0.4.3 backwards compatibility Feb 9, 2024
@errnoh
Copy link
Contributor Author

errnoh commented Feb 9, 2024

Though the health check not working in this setup should probably be fixed as well. Is it using python found from g:python3_host_prog or just checking for any python binary? Neovim in NixOS is often using wrapped python that's named nvim-python3 so maybe that's why it's reporting that it's finding the other binary instead?

@jellydn jellydn changed the base branch from main to canary February 9, 2024 15:58
@jellydn
Copy link
Contributor

jellydn commented Feb 9, 2024

Great. Thanks @errnoh. I've just changed this PR to canary branch instead of main.

Let's see if @gptlang and @ziontee113 have any comments/thoughts on this PR.

IMO, I could merge this to canary and test on my side as well.

@errnoh
Copy link
Contributor Author

errnoh commented Feb 9, 2024

Added one more commit to ensure that the correct python binary is being used and that it reports >=0.4.3 as valid versions.

@gptlang
Copy link
Member

gptlang commented Feb 9, 2024

In what versions does relative imports not work? I really don't like import *s

Could adding __init__.pys to every directory help it identify the fact that it's a package?

@errnoh
Copy link
Contributor Author

errnoh commented Feb 11, 2024

Took a look at this with fresh pair of eyes and figured out a simple way to get best of both worlds. Imports are now fixed in the latest commit and are 1:1 with the original code but just added package declaration at the start of the import.

@jellydn
Copy link
Contributor

jellydn commented Feb 12, 2024

Thanks @errnoh Could you fix the conflict?

image

@errnoh
Copy link
Contributor Author

errnoh commented Feb 12, 2024

Thanks @errnoh Could you fix the conflict?

Fixed 👍 @jellydn

Copy link
Contributor

@jellydn jellydn left a comment

Choose a reason for hiding this comment

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

LGTM!

@jellydn jellydn merged commit b480c96 into CopilotC-Nvim:canary Feb 13, 2024
cjoke pushed a commit to cjoke/CopilotChat.nvim that referenced this pull request Feb 18, 2024
* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

---------

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
cjoke pushed a commit to cjoke/CopilotChat.nvim that referenced this pull request Feb 18, 2024
* Intergrate ziontee113/CopilotAgent.nvim (CopilotC-Nvim#27)

* feat: add libraries from ziontee113/CopilotAgent

* feat: add chat handlers from CopilotAgent

* feat: add "mycopilot" module from CopilotAgent

* feat: add "tools" module from CopilotAgent

* feat: add copilot-agent.py

* feat: conditionally check for tiktoken module

* Add renamed CopilotAgent commands to README.md (CopilotC-Nvim#29)

* ref: rename commands to prefixed with CopilotChat

* docs: add renamed CopilotAgent commmands

* docs: add default key mapping

---------

Co-authored-by: Dung Duc Huynh (Kaka) <[email protected]>

* docs: add note about the canary branch

docs: fix keymap for in-place command

* chore: update dict

* docs: add demo videos for Fold & Inplace (CopilotC-Nvim#31)

* chore: remove testing commands and layout component

fix: remove matrix testing code

* docs: remove copilot chat vspilt command

* feat: add toggle layout and new split preset commands

* revert: add internal comands back for inplace chat

* chore: add a note about chat handler

* feat: show spinner on processing

* feat: show key binding on help section

* docs: fix usage for InPlace command

* docs: add demo for token count and demo

* feat: add C-h to toggle help section

* docs: add usage for InPlace command

* chore: sync fork

* refactor: create dedicated function for help text with layout

* feat: add option for hide or show the help text on InPlace

* Fix module import error by using typing.List (CopilotC-Nvim#33)

Co-authored-by: Cassius0924 <[email protected]>

* docs: add Cassius0924 as a contributor for code (CopilotC-Nvim#34)

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

---------

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* feat: add user prompt and clear history on InPlace command

* docs: fix usage for install manually

* feat: add new keymap for change system prompt

* feat: add debug flag

* docs: add debug file path when enable debug mode

* docs: notify end user to run UpdateRemotePlugins command after install plugin

* chore: sync fork

Add system prompt to ask
Handle bad error code after calling post

* chore: sync fork and remove unused

* docs: add Discord link

* chore: sync fork

* refactor!: drop CC commands

* docs: remove branch on usage

---------

Co-authored-by: Trí Thiện Nguyễn <[email protected]>
Co-authored-by: He Zhizhou <[email protected]>
Co-authored-by: Cassius0924 <[email protected]>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants