jackMort / ChatGPT.nvim

ChatGPT Neovim Plugin: Effortless Natural Language Generation with OpenAI's ChatGPT API
Apache License 2.0
3.57k stars 307 forks source link

Fix #303: curl error #305

Closed ekaj2 closed 9 months ago

ekaj2 commented 9 months ago

curl: option -H: requires parameter error

Seems to be related to this change here from this PR that tries to add support for AZURE environment variables as well.

https://github.com/jackMort/ChatGPT.nvim/pull/293/files

Api.AUTHORIZATION_HEADER is nil in Api.chat_completions...

The callback passed to loadApiKey is not guaranteed to be called in the case that the environment variable for OPENAI_API_KEY exists.

local function loadConfigFromEnv(envName, configName)
  local variable = os.getenv(envName)
  if not variable then
    return
  end
  Api[configName] = variable:gsub("%s+$", "")
end

-- ...

local function loadApiKey(envName, configName, optionName, callback, defaultValue)
  loadConfigFromEnv(envName, configName)
  if not Api[configName] then
    if Config.options[optionName] ~= nil and Config.options[optionName] ~= "" then
      loadConfigFromCommand(Config.options[optionName], optionName, callback, defaultValue)
    else
      logger.warn(envName .. " environment variable not set")
      return
    end
  end
end

A simple solution is to pass the callback function in to handle handle if the environment variable is picked up in the config. I'm making a PR for that now.

jackyu1996 commented 9 months ago

@ekaj2 In this PR, since not all loadConfigFromEnv calls are updated to include a callback function, the callback passed can be nil. Could you please make the necessary changes for them? More specifically here and here. If possible, maybe loading the OPENAI_API_TYPE can also be modified to support reading from command. Thanks a lot!

ekaj2 commented 9 months ago

@jackyu1996 Not sure I'll have any more time this weekend, but I'll hopefully come back to that over the week. What we really need is some sort of unit testing. It's already quite hard to manually check all routes here.

Also a refactor of this code so it's not a bunch of params and callbacks getting passed all the way down would help reduce the bugs

jackyu1996 commented 9 months ago

@ekaj2 Sure, thank you all the same, and I totally agree with your suggestions. Have a nice weekend!

rzalawad commented 9 months ago

@ekaj2 @jackyu1996 I've raised the PR https://github.com/jackMort/ChatGPT.nvim/pull/306.