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

Feat/ai/explain #754

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Feat/ai/explain #754

wants to merge 10 commits into from

Conversation

BBerabi
Copy link
Contributor

@BBerabi BBerabi commented Jan 16, 2025

Description

Provide description of this PR and changes, if linked Jira ticket doesn't cover it in full.

Checklist

  • Tests added and all succeed
  • Linted
  • README.md updated, if user-facing
  • License file updated, if new 3rd-party dependency is introduced

return nil, errors.New("failed to parse diff")
}

explanation, err := cmd.codeScanner.GetAIExplanation(ctx, derivation, ruleKey, ruleMessage, diff)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will go somewhere else - most likely in a new subcomponent (SnykLLMBindings)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file would basically become DeepCodeLLMBindings

return ExplainResponse{}, err
}

url := "http://localhost:10000/explain"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this should be injected from configuration

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be the same API endpoint that we have from the config.Endpoint?

Copy link
Collaborator

Choose a reason for hiding this comment

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

move to DeepCodeLLMBinding

Comment on lines +39 to +45
type ExplainOptions struct {
derivation string
ruleKey string
ruleMessage string
diff string
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

this would also be separated out into the DeepCodeLLMBindings

Comment on lines +91 to +96

GetAIExplanation(ctx context.Context, options ExplainOptions) (
explanation string,
status string,
err error,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this, too

@@ -237,6 +237,19 @@ <h2 class="example-fixes-header">Fixed Code Examples</h2>
<!-- AI Fix -->
<div id="ai-fix-wrapper" class="{{if not .HasAIFix}}hidden{{end}}">
<!-- AI fix buttons -->
<section id="explain-wrapper" class="ai-explain">
<h2 class="ai-fix-header">
DeepCode AI Explanations
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should become a variable, so we could potentially support ANY LLMBinding for explanations

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, we should even provide a separate LLM explain panel for the explanation @andygongea

Copy link
Collaborator

Choose a reason for hiding this comment

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

These types should be generalized, to support different LLMBindings

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.

3 participants