-
Notifications
You must be signed in to change notification settings - Fork 52
Add endpoints for interacting with env tags in ts. #1229
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
base: main
Are you sure you want to change the base?
Conversation
ibolmo
left a comment
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.
left a comment on dx and another on error handling
| public listPromptEnvironments( | ||
| options: ListPromptEnvironmentsOptions, | ||
| ): Promise<PromptEnvironmentAssociation[]> { | ||
| return listPromptEnvironments(options); |
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'd expect an Environments class with a list, set, clear.
something like
prompt.environments.list() and so on
This would follow existing prompt.project etc.
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'd imagine this environment class would be used in logger instead of the function based approach
| return id; | ||
| } | ||
|
|
||
| public listPromptEnvironments( |
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.
not sure i'd expect these methods here if the framework2 already has this. 🤔
if we want to keep them would imagine something like:
getEnvironments()
setEnvironments()
clearEnvironments()
(Prompt seemed duplicative)
| forceLogin, | ||
| }); | ||
|
|
||
| const response = await state |
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.
missing try catch? perhaps better error message
| orgName ? { org_name: orgName } : {}, | ||
| ); | ||
|
|
||
| return (response.objects ?? []) as PromptEnvironmentAssociation[]; |
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.
would this hide an upstream error?
|
|
||
| // Use PUT for upsert behavior - need to use state.fetch since HTTPConnection doesn't have put method | ||
| const apiConn = state.apiConn(); | ||
| const url = `${apiConn.base_url}/environment-object/prompt/${promptId}/${environmentSlug}`; |
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.
should we just fix this at the apiConn level? i.e. add a put_json()
| const apiConn = state.apiConn(); | ||
| const queryString = orgName ? `?org_name=${encodeURIComponent(orgName)}` : ""; | ||
| const url = `${apiConn.base_url}/environment-object/prompt/${promptId}/${environmentSlug}${queryString}`; | ||
| const resp = await state.fetch(url, { |
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.
ditto here
Test script: