-
Notifications
You must be signed in to change notification settings - Fork 217
Add "View Autofixes" feature for variant analysis results #4065
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
Conversation
This function will increment file numbers when running autofix in a loop
8095989 to
c5f0e5e
Compare
c5f0e5e to
3ff5d64
Compare
extensions/ql-vscode/src/variant-analysis/variant-analysis-view.ts
Outdated
Show resolved
Hide resolved
Reference codeQL.autofix.path setting instead of env var
@aeisenberg Which comment are you referring to? There were four things I still was planning to address: three comments related to editing parts of |
|
Oh...there might be more things that I missed. I'm out tomorrow. I think it's also a holiday in the US, so I can take another look on Monday. |
|
@aeisenberg @koesie10 I've addressed all review comments. Let me know if you want any other changes, especially regarding the environment variables. |
koesie10
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.
Thanks, this looks good to me. I have some optional comments, but I don't think any of those are necessary.
| // Get autofix binary. | ||
| // Switch to Go binary in the future and have user pass full path | ||
| // in an environment variable instead of hardcoding part here. | ||
| const cocofixBin = join(process.cwd(), localAutofixPath, "bin", "cocofix.js"); |
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.
If the Go binary accepts the same parameters as cocofix, would it makes sense to already add a config setting for this and use a default of bin/cocofix.js?
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 need to experiment with the Go binary manually. I'm not sure yet if it accepts all of the same parameters. I'll likely edit this into a config setting when I create a follow-up PR for switching to Go.
| env: { | ||
| CAPI_DEV_KEY: process.env.CAPI_DEV_KEY, | ||
| PATH: process.env.PATH, | ||
| }, |
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 don't think this is really necessary if it's unlikely that this will be used. I haven't run autofix myself locally, so I'm not sure if it depends on any environment variables.
aeisenberg
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.
Looks generally good. I see one area where I think you can simplify things.
|
@koesie10 @aeisenberg I've responded to all new review comments. Thanks for the thorough reviews! 🙇 |
koesie10
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.
LGTM, thanks!
aeisenberg
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.
Great job!
I haven't tried this locally, but the code looks good. It's likely that the failing E2E job is transient. I'll kick it off again and hopefully it will work.
|
@aeisenberg There's a known issue with the E2E tests failing since end of March. The issue tracking it is in the private code-scanning repo. So I think this PR can be merged despite that test failure? |
|
I'm fine with that. |
Description
Adds a "View Autofixes" feature for variant analysis results. This is a canary-only feature.
Generates autofixes for selected variant analysis results and opens the resulting autofix outputs in a markdown file.
Activated via the "View Autofixes" button in the variant analysis webview or via right-clicking the variant analysis query history item.
At a high-level, the implementation:
Commit-by-commit review recommended. Let me know if you want me to split anything onto a separate PR.
Consideration
queryHelpOverrideDirectoryto the newgo/pkg/autofix/prompt/qhelpsdirectory in that PR.Testing