Conversation
There was a problem hiding this comment.
Pull request overview
Removes dead/unused functions from the parser import system and updates call sites (mostly tests) to use the remaining ProcessImportsFromFrontmatterWithSource API.
Changes:
- Removed dead
ImportError.Error()/ImportError.Unwrap()methods and deleted wrapper import-processing functions from production code. - Updated tests to call
ProcessImportsFromFrontmatterWithSourcedirectly (and introduced a local test helper where needed). - Deleted now-obsolete unit test that only exercised the removed
ImportError.Error()behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/import_processor.go | Removes the exported wrapper functions, leaving ProcessImportsFromFrontmatterWithSource as the entry point. |
| pkg/parser/import_error.go | Removes dead ImportError methods (Error, Unwrap). |
| pkg/parser/import_error_test.go | Removes unit test that only covered the deleted ImportError.Error() method. |
| pkg/parser/frontmatter_utils_test.go | Adds a local test helper to preserve the old wrapper behavior for this test file. |
| pkg/parser/yaml_import_test.go | Updates YAML-import tests to call ProcessImportsFromFrontmatterWithSource. |
| pkg/parser/yaml_import_e2e_test.go | Updates E2E YAML-import tests to call ProcessImportsFromFrontmatterWithSource. |
| pkg/parser/yaml_import_copilot_setup_test.go | Updates Copilot setup import test to call ProcessImportsFromFrontmatterWithSource. |
| pkg/parser/import_topological_test.go | Updates topological ordering tests to call ProcessImportsFromFrontmatterWithSource. |
| pkg/parser/import_topological_contract_test.go | Updates contract tests to call ProcessImportsFromFrontmatterWithSource. |
| pkg/parser/import_remote_nested_test.go | Updates remote/nested import test to call ProcessImportsFromFrontmatterWithSource. |
| pkg/parser/agent_import_integration_test.go | Updates agent import integration tests to call ProcessImportsFromFrontmatterWithSource. |
| pkg/cli/dependency_graph_test.go | Updates dependency-graph contract test to call ProcessImportsFromFrontmatterWithSource. |
Comments suppressed due to low confidence (1)
pkg/parser/frontmatter_utils_test.go:552
- The test now calls the local helper processImportsFromFrontmatter, but several failure messages still refer to ProcessImportsFromFrontmatter() (which has been removed from production). Consider updating the strings to reference the helper / current API to avoid confusing test output.
tools, engines, err := processImportsFromFrontmatter(tt.frontmatter, tempDir)
if tt.wantErr {
if err == nil {
t.Errorf("ProcessImportsFromFrontmatter() expected error but got none")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // runImportProcessor processes the given top-level import list via | ||
| // ProcessImportsFromFrontmatterWithManifest and returns the ImportedFiles slice. | ||
| func runImportProcessor(t *testing.T, dir string, topImports []string) []string { | ||
| t.Helper() | ||
| fm := map[string]any{"imports": topImports} | ||
| result, err := parser.ProcessImportsFromFrontmatterWithManifest(fm, dir, nil) | ||
| result, err := parser.ProcessImportsFromFrontmatterWithSource(fm, dir, nil, "", "") | ||
| require.NoError(t, err, "ProcessImportsFromFrontmatterWithManifest must not fail") | ||
| return result.ImportedFiles |
There was a problem hiding this comment.
The helper comment and assertion message still refer to ProcessImportsFromFrontmatterWithManifest, but the helper now calls ProcessImportsFromFrontmatterWithSource. Please update this comment/message so the contract test documentation and failure output remain correct.
See below for a potential fix:
// ProcessImportsFromFrontmatterWithSource and returns the ImportedFiles slice.
func runImportProcessor(t *testing.T, dir string, topImports []string) []string {
t.Helper()
fm := map[string]any{"imports": topImports}
result, err := parser.ProcessImportsFromFrontmatterWithSource(fm, dir, nil, "", "")
require.NoError(t, err, "ProcessImportsFromFrontmatterWithSource must not fail")
Removes 4 dead functions from the parser import system:
ImportError.Error()andImportError.Unwrap()— struct is still used but never cast toerrorinterface; methods were dead from production pathsProcessImportsFromFrontmatter— thin wrapper now only needed in one test file, moved to local test helperProcessImportsFromFrontmatterWithManifest— thin wrapper; all 16 test call sites updated to callProcessImportsFromFrontmatterWithSourcedirectlyPart of systematic dead code cleanup (DEADCODE.md phase 8).