frankroeder / parrot.nvim

parrot.nvim 🦜 - the plugin that brings stochastic parrots to Neovim. This is a gp.nvim-fork focused on simplicity.
Other
219 stars 14 forks source link

Refactor config and integrate tests #23

Closed frankroeder closed 2 months ago

eterps commented 2 months ago

@frankroeder , would it be okay for you if we move the helper functions to a separate module file like config_utils.lua?

I think it could be helpful in making things easier to test. Currently I am testing changes by restarting nvim after every change and logging things, which is quite cumbersome.

A separate file like config_utils.lua could enable easy automated testing using plenary-test, which could look like f.i.:

describe('config_utils', function()
  describe('merge_providers', function()
    it('should merge default and user providers', function()
      local default_providers = {
        pplx = {
          api_key = '',
          endpoint = 'https://api.perplexity.ai/chat/completions',
          topic_prompt = 'default prompt',
          topic_model = 'llama-3-8b-instruct',
        },
        ollama = {
          endpoint = 'http://localhost:11434/api/chat',
          topic_prompt = 'Summarize the chat above in max 3 words',
          topic_model = 'mistral:latest',
        },
      }
      local user_providers = {
        pplx = { api_key = '123' },
        ollama = { endpoint = 'http://localhost:8000/api/chat' },
      }

      local result = config_utils.merge_providers(default_providers, user_providers)

      assert.are.same({
        pplx = {
          api_key = '123',
          endpoint = 'https://api.perplexity.ai/chat/completions',
          topic_prompt = 'default prompt',
          topic_model = 'llama-3-8b-instruct',
        },
        ollama = {
          endpoint = 'http://localhost:8000/api/chat',
          topic_prompt = 'Summarize the chat above in max 3 words',
          topic_model = 'mistral:latest',
        },
      }, result)
    end)
  end)

This way we could capture complexity in simple examples and making testing 'by hand' unnecessary. Also it would be known when existing behaviour would be broken at some point in the future.

frankroeder commented 2 months ago

@eterps I already thought about testing and you finally initiated it with this PR.

frankroeder commented 2 months ago

@eterps , could you please double-check the additions I’ve made and confirm if everything is functioning as it should? I’ve been using this branch daily and haven’t encountered any problems, except for one case. If you transition from the previous version where all providers are enabled by default, this branch will generate a few errors when trying to access certain values stored in the state.json file. One possible solution could be to suggest deleting the state.json file.

For any further changes, it would be best to create a new issue or pull request. It would also be helpful if we could outline the next steps somewhere.

eterps commented 2 months ago

@frankroeder I have seen your additions, they look good to me. I also have been on this branch daily without problems.

If you transition from the previous version where all providers are enabled by default, this branch will generate a few errors when trying to access certain values stored in the state.json file. One possible solution could be to suggest deleting the state.json file.

While deleting the state.json file might work as a solution, it could also indicate an actual bug. It's possible that the errors also occur when someone stops using a certain provider in their user config. I can check tomorrow whether that is the case.

For any further changes, it would be best to create a new issue or pull request.

I agree. So far the process has been a bit messy for me, but I learned a great deal about Lua and also about nvim plugin development.

It would also be helpful if we could outline the next steps somewhere.

Are you referring to further cleanup or the planning of new features to add?

With regard to new features I think we can get inspiration from:

Additionally, I recently learned about rocks.nvim. If this approach becomes successful, it might be interesting to publish parrot.nvim on LuaRocks because then it would be the first Neovim LLM integration plugin that can be installed from rocks.nvim.

eterps commented 2 months ago

With respect to my earlier comment:

Are you referring to further cleanup or the planning of new features to add?

I think we still need to fix the system prompt issue with Anthropic (i.e. giving the provider configurations the same structure for end-users).

Because I just discovered a bug. If you activate a custom command using Anthropic provider and switch back to a general chat window, the system prompt keeps the value of the (previously activated) custom command (messing up the general chat system prompt).

eterps commented 2 months ago

Another useful feature is being able to customise assitant prompt (i.e. agent prefix/suffix) in the responses for custom commands (for example my 'proofreader' command could have a different emoji like 👓 instead of 🦜).