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

[vscode][7/n] Setup Env Variables: Connect missing API keys error to command #1300

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

rossdanlm
Copy link
Contributor

@rossdanlm rossdanlm commented Feb 22, 2024

[vscode][7/n] Setup Env Variables: Connect missing API keys error to command

Now it's more intuitive for users to know what needs to be done when they receive this error. I also moved the logic for the command into a utils file so it can be accessed both by activation flow and the error checking flow in the editor

Test Plan

Getting missing API key error

559b2a29-b906-46c8-815b-2f90b0ce9299.mp4

Walkthrough commands

c0dd6d72-4fd3-4c51-ac84-9ae76a3e9cd7.mp4

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link
Contributor

@rholinshead rholinshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just suggestion to use markdown for link to docs if it'll work

// logic is defined in https://github.com/lastmile-ai/aiconfig/blob/33fb852854d0bd64b8ddb4e52320112782008b99/python/src/aiconfig/util/config_utils.py#L41
if (message.includes("Missing API key")) {
notificationAction = await notificationFn(
"Looks like you're missing an API key, please set it in your env variables",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I found out recently is that vscode has MarkdownString which can be used in some vscode components. If it works, would be great to use to show documentation link here as well:

  const message = new vscode.MarkdownString();
  message.appendMarkdown(
    "Looks like you're missing an API key. Please set it in your <a href='https://aiconfig.lastmileai.dev/docs/getting-started/#setup-your-api-keys' target='_blank'>.env variables</a>"
  );
  message.isTrusted = true; // allow links to be clickable -- not sure if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried getting this to work but wasn't able to for diagnostics messaging. Seems like it's still an open issue: microsoft/vscode#54272

rossdanlm added a commit that referenced this pull request Feb 22, 2024
…put (#1291)

[vscode][6/n] Setup Env Variables: Add more validation to env path input

Added two noteworthy checks that we didn't have before:

1. The filename itself must be ".env" (before it only checked for ending
so things like "invalid/filename.env" could be valid)
2. The path must be a subset of the workspace path, to ensure that when
we call the `loadenv()` function from Python in AIConfig SDK, it's able
to access this `.env` file

I also made this into a separate function to keep it cleaner

## Test Plan


https://github.com/lastmile-ai/aiconfig/assets/151060367/d77305c4-2cc6-4cb8-a918-b74826ff3714

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/1291).
* #1300
* __->__ #1291
…command

Now it's more intuitive for users to know what needs to be done when they receive this error. I also moved the logic for the command into a utils file so it can be accessed both by activation flow and the error checking flow in the editor

## Test Plan

Getting missing API key error

https://github.com/lastmile-ai/aiconfig/assets/151060367/cc6c51e5-d205-4afa-a3b4-dc04f68d4946

Walkthrough commands

https://github.com/lastmile-ai/aiconfig/assets/151060367/bda40b3c-d260-40a2-82da-563d363152b3
@rossdanlm
Copy link
Contributor Author

Updated content a bit

Screenshot 2024-02-22 at 17 45 53

@rossdanlm rossdanlm merged commit d7821eb into main Feb 22, 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.

2 participants