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

Fix #303: curl error #305

Merged
merged 1 commit into from
Oct 15, 2023
Merged

Fix #303: curl error #305

merged 1 commit into from
Oct 15, 2023

Conversation

ekaj2
Copy link
Contributor

@ekaj2 ekaj2 commented Oct 14, 2023

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.

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.
@jackMort jackMort merged commit a5448ee into jackMort:main Oct 15, 2023
1 check passed
@jackyu1996
Copy link
Contributor

@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
Copy link
Contributor Author

ekaj2 commented Oct 15, 2023

@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
Copy link
Contributor

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

@rzalawad
Copy link
Contributor

@ekaj2 @jackyu1996 I've raised the PR #306.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants