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 improvement to whichapi & anthropic configuration #42

Merged
merged 8 commits into from
Sep 30, 2024

Conversation

DaveHogan
Copy link
Contributor

I made a typo when configuring my whichApi value🙄

Hopefully, this PR prevents others doing the same by forcing the values to be selectable from an enum.

Whilst in here, I made the max_tokens and model values for anthropic a configurable value.

I'm happy to implement similar approach on the others configs and refactor the configuration a little but I didn't want to jump in without running by the void owners. I would also suggest waiting until PR #22 is implemented as that introduces some new configuration values for Ollama integration and don't want to raise a bunch of conflicts

Makes whichApi an enum for less error prone configuration
@okxiaoliang4
Copy link
Contributor

okxiaoliang4 commented Sep 21, 2024

relate:

when #41 merged i will improve the configuration in this pr okxiaoliang4#1

iShot_2024-09-21_12 43 23

@okxiaoliang4 okxiaoliang4 mentioned this pull request Sep 21, 2024
5 tasks
@andrewpareles
Copy link
Contributor

We shouldn't have ?? 'anthropic' or ?? 1024. These are already defaults in package.json; it doesn't make sense to hard-code them in twice.

Please make these all default the empty string, and parse values where they're used.

@DaveHogan
Copy link
Contributor Author

Yeah that's fair, but if I was to parse the values like below should we have a fallback or alert the user in some manner?

const parseWhichApi = (value: any): string => {
	if (typeof value === 'string' && ['anthropic', 'openai', 'greptile', 'ollama'].includes(value)) {
		return value;
	}
	// What to do in the negative cases here?
	return 'anthropic'; 
}

Also maxTokens is a number, would you suggest converting that to string or keeping as a number with 0 as a default

parseMaxTokens(config.get('anthropicMaxToken')) ?? 0

@andrewpareles
Copy link
Contributor

To keep everything consistent, I suggest making everything a string. Let’s parseInt() when we need an int.

I think we should just raise an error in the “negative” case. This makes it much clearer what’s going on in the case of an unexpected value. Also, I’m pretty sure the negative case of whichApi can never be reached anyway - package.json says the value is an enum.

@DaveHogan
Copy link
Contributor Author

I've rejigged this from your suggestions @andrewpareles

@andrewpareles andrewpareles merged commit 3fd9b57 into voideditor:main Sep 30, 2024
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