60 lines
2.1 KiB
Markdown
60 lines
2.1 KiB
Markdown
|
# PR #283 Review: Use property descriptions for completion items
|
||
|
|
|
||
|
|
## Summary
|
||
|
|
|
||
|
|
This PR fixes a bug where completion items for action.yml were missing descriptions. The root cause was that `mappingValues()` only looked at the type definition's description, ignoring property-level descriptions in the schema.
|
||
|
|
|
||
|
|
## Changes Analysis
|
||
|
|
|
||
|
|
### Core Fix ([definition.ts](languageservice/src/value-providers/definition.ts))
|
||
|
|
|
||
|
|
**Before:**
|
||
|
|
```typescript
|
||
|
|
let description: string | undefined;
|
||
|
|
if (value.type) {
|
||
|
|
const typeDef = definitions[value.type];
|
||
|
|
description = typeDef?.description;
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
**After:**
|
||
|
|
```typescript
|
||
|
|
let description: string | undefined = value.description;
|
||
|
|
if (value.type) {
|
||
|
|
const typeDef = definitions[value.type];
|
||
|
|
if (!description) {
|
||
|
|
description = typeDef?.description;
|
||
|
|
}
|
||
|
|
}
|
||
|
|
```
|
||
|
|
|
||
|
|
✅ **Correct approach** - prioritizes property description, falls back to type description.
|
||
|
|
|
||
|
|
### Test Coverage
|
||
|
|
|
||
|
|
1. **complete-action.test.ts**: Two new tests verify `author` and `branding` completions include documentation
|
||
|
|
2. **hover-action.test.ts**: New test for `author` hover + updated `branding` test to verify "Documentation" link
|
||
|
|
|
||
|
|
## Potential Issues
|
||
|
|
|
||
|
|
### 1. One-of expansion doesn't use property description
|
||
|
|
|
||
|
|
Looking at line 140-142:
|
||
|
|
```typescript
|
||
|
|
const expanded = expandOneOfToCompletions(oneOfDef, definitions, key, description, indentation, mode);
|
||
|
|
```
|
||
|
|
|
||
|
|
This passes `description` to `expandOneOfToCompletions`, but at this point `description` may have been populated from the property. **This is correct** - the property description is passed through.
|
||
|
|
|
||
|
|
### 2. Consistency check
|
||
|
|
|
||
|
|
The PR description mentions this is consistent with hover. Verified: [template-reader.ts#L225](workflow-parser/src/templates/template-reader.ts#L225) shows hover uses `nextPropertyDef.description` when available.
|
||
|
|
|
||
|
|
## Verdict
|
||
|
|
|
||
|
|
✅ **LGTM** - Clean, minimal fix that aligns completion behavior with hover. Good test coverage for the specific cases mentioned.
|
||
|
|
|
||
|
|
## Minor Suggestions (non-blocking)
|
||
|
|
|
||
|
|
1. Could add a test for a property that has NO description but whose type DOES have one, to verify fallback works (e.g., `inputs` which references `inputs-strict` type that has a description)
|