-
Notifications
You must be signed in to change notification settings - Fork 51
Setup CodeActions and add quickfix for missing inputs #254
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
c66abfd to
b6bd0ad
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.
Pull request overview
This PR introduces CodeAction capabilities to the GitHub Actions language server, specifically implementing a quickfix feature for automatically adding missing required action inputs. The implementation includes:
- A new code action infrastructure with a provider-based architecture
- A quickfix provider that generates edits to add missing required inputs
- Test infrastructure using golden files to validate code action behavior
- Enhanced diagnostic data to support code actions
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
languageservice/src/code-actions/index.ts |
Main entry point for code actions, implementing provider aggregation and filtering logic |
languageservice/src/code-actions/types.ts |
Type definitions for code action context and provider interface |
languageservice/src/code-actions/quickfix/index.ts |
Exports quickfix providers array |
languageservice/src/code-actions/quickfix/add-missing-inputs.ts |
Implementation of the quickfix provider for adding missing required inputs |
languageservice/src/code-actions/tests/runner.ts |
Test infrastructure for running golden file tests |
languageservice/src/code-actions/tests/runner.test.ts |
Test suite using the golden file runner |
languageservice/src/code-actions/tests/testdata/quickfix/*.yml |
Test input files with markers for expected diagnostics and fixes |
languageservice/src/code-actions/tests/testdata/quickfix/*.golden.yml |
Expected output files after applying code actions |
languageservice/src/validate-action.ts |
Enhanced validation to include diagnostic data needed for code actions |
languageservice/src/validate.actions.test.ts |
Updated tests to verify new diagnostic data fields |
languageservice/src/index.ts |
Exports the new getCodeActions function |
languageserver/src/connection.ts |
Registers code action handler and declares CodeActionKind.QuickFix capability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
languageservice/src/code-actions/quickfix/add-missing-inputs.ts
Outdated
Show resolved
Hide resolved
81fed48 to
1b0264b
Compare
1b0264b to
f35d847
Compare
| const inputLines = data.missingInputs.map(input => { | ||
| const value = input.default !== undefined ? input.default : '""'; | ||
| return `${inputIndent}${input.name}: ${value}`; | ||
| }); |
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 probably refactor this, looks the exact same as lines 53-36
| const inputLines = data.missingInputs.map(input => { |
| const inputIndent = " ".repeat(data.stepIndent + 2); | ||
|
|
||
| const inputLines = data.missingInputs.map(input => { | ||
| const value = input.default !== undefined ? input.default : '""'; |
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.
cleaner to write if "" (empty string) is not a valid input
| const value = input.default !== undefined ? input.default : '""'; | |
| const value = input.default ?? '""'; |
| } | ||
| }; | ||
|
|
||
| function createInputEdits(data: MissingInputsDiagnosticData): TextEdit[] | undefined { |
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 like this can only return TextEdit[], not sure we need undefined here unless I'm mistaken
| function createInputEdits(data: MissingInputsDiagnosticData): TextEdit[] | undefined { | |
| function createInputEdits(data: MissingInputsDiagnosticData): TextEdit[] { |
|
|
||
| if (data.hasWithKey && data.withIndent !== undefined) { | ||
| // `with:` exists - use its indentation + 2 for inputs | ||
| const inputIndent = " ".repeat(data.withIndent + 2); |
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 put this as a consant somewhere, I see + 2 is used quite a bit, I think we can put something like const YAML_INDENT = 2;.
| return `${inputIndent}${input.name}: ${value}`; | ||
| }); | ||
|
|
||
| const newText = [`${withIndent}with:\n`, ...inputLines.map(line => `${line}\n`)].join(""); |
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.
nit I think we can simplify to this but double check, though you can ignore these, looks like the exact same
| const newText = [`${withIndent}with:\n`, ...inputLines.map(line => `${line}\n`)].join(""); | |
| const newText = `${withIndent}with:\n` + inputLines.map(line => `${line}\n`).join(""); |
|
|
||
| export interface CodeActionContext { | ||
| uri: string; | ||
| // TODO: add things like workflow template, parsed content, 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.
We may want to be careful about adding more things here, since removing or changing the structure would be a compat break
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.
Wondering if we should remove this TODO ?
|
|
||
| if (data.hasWithKey && data.withIndent !== undefined) { | ||
| // `with:` exists - use its indentation + 2 for inputs | ||
| const inputIndent = " ".repeat(data.withIndent + 2); |
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.
Instead of assuming 2, have you considered matching the customer's indent? You could detect this based on the indent amount of the first job. For example:
function detectIndentSize(rootToken: MappingToken): number {
const jobsValue = rootToken.find("jobs");
if (jobsValue && isMapping(jobsValue) && jobsValue.count > 0) {
const firstJob = jobsValue.get(0);
if (firstJob?.key.range && jobsValue.range) {
return firstJob.key.range.start.column - jobsValue.range.start.column;
}
}
return 2; // fallback
}
This PR adds CodeAction capabilities to the language server and sets up the inner loop for them, using workflow files where to define expected diagnostics and golden files to specify expected results. This also adds a
CodeActionKind.QuickFixfor when an action is used but it's missing required inputs: