-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor config and integrate tests #23
Conversation
@frankroeder , would it be okay for you if we move the helper functions to a separate module file like I think it could be helpful in making things easier to test. A separate file like 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. |
@eterps I already thought about testing and you finally initiated it with this PR. |
Refactor config merge
- adjust comment
- change keybinding descriptions
@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. |
@frankroeder I have seen your additions, they look good to me. I also have been on this branch daily without problems.
While deleting the
I agree.
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 |
- add an appropriate plenary test - fix state_spec test state file names
With respect to my earlier comment:
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). |
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 🦜). |
No description provided.