-
Notifications
You must be signed in to change notification settings - Fork 13
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
docs: add contact page, improve tables and examples #56
Conversation
b691a32
to
db6af59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very opinionated on the main readme, see my comment. Also I don't think we should be opinionated on the use of dotenv
to load the environment variables because in most situations it doesn't make sense to use.
const messages: ChatCompletionMessageParam = [{ | ||
role: 'user', | ||
content: `How are you?`, | ||
}] | ||
|
||
// Call the create function | ||
const result = await tokenjs.chat.completions.create({ | ||
// Specify the target model and provider | ||
provider: 'ai21', | ||
model: 'jamba-instruct', | ||
messages, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of defining the messages array separately was to demonstrate the use of types and that you can import the types from our package which is a nice quality of life thing IMO.
I'm not super opinionated on this, but I do think removing it makes the example slightly worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very opinionated on this either, but I'm still getting a type error, so I don't think we should consider including it in the example until that's resolved. I opened an issue for it here: #67
Want me to create a separate ticket for adding the ChatCompletionsMessageParam
type back to the documentation examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I would bring back the message types but I'm not so opinionated as to block the PR on it.
messages
array in thecreate
function instead of assigning it to a variable because there's currently a TypeScript type error whenmessages
is assigned to a variablemain
function so that users can copy and paste the example into a file and run it as is