Conversation
Replace if-else chain with switch statement (gocritic) and use strings.Join instead of string concatenation in a loop (modernize).
Add 8 txtar integration tests covering: - Clean project (no issues found, exit 0) - Pydantic BaseModel detection (exit 1) - Pydantic BaseModel auto-fix with --fix - Deprecated imports detection (exit 1) - Deprecated imports auto-fix with --fix - Missing predict class reference (exit 1) - Exit code 1 when predict file is missing - Deprecated config fields as warnings (exit 0)
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Adds a cog doctor command to diagnose and auto-fix common project issues. The implementation is solid with good test coverage and follows existing patterns.
One potential issue with error handling in runner.go:
| } | ||
|
|
||
| parser := sitter.NewParser() | ||
| parser.SetLanguage(python.GetLanguage()) |
There was a problem hiding this comment.
The context.Background() here creates a new root context, ignoring any cancellation signals from the caller. Since the Run function receives a context parameter, consider using that instead:
| parser.SetLanguage(python.GetLanguage()) | |
| tree, err := parser.ParseCtx(ctx, nil, source) |
|
I've submitted my review on PR #2923 with one suggestion about using the passed context instead of |
|
LGTM |
…ate config loading, improve fix robustness Critical fixes: - HasErrors() now accounts for check internal errors (exit code 1) - hasArbitraryTypesAllowed uses tree-sitter AST instead of string matching (no false positives) - addToCogImport handles missing 'from cog import' line - Config loading consolidated onto CheckContext (eliminates 4x redundant Load calls) High priority fixes: - Respect --file flag instead of hardcoding cog.yaml - Handle 'import pydantic' module-level style in fix - Deprecated imports fix uses tree-sitter re-scan instead of fragile string matching - ConfigPredictRefCheck reuses cached Python files from ctx.PythonFiles - model_config fix preserves other ConfigDict kwargs Medium fixes: - Docker/Python exec commands have 5-second timeout - Multi-line parenthesized imports handled by fix - File permissions preserved (not hardcoded to 0o644) - printDoctorResults counts per-finding consistently
|
|
||
| parser := sitter.NewParser() | ||
| parser.SetLanguage(python.GetLanguage()) | ||
| tree, err := parser.ParseCtx(context.Background(), nil, source) |
There was a problem hiding this comment.
The context.Background() here creates a new root context, ignoring any cancellation signals from the caller. Since the Run function receives a context parameter, consider using that instead:
| tree, err := parser.ParseCtx(context.Background(), nil, source) | |
| tree, err := parser.ParseCtx(ctx, nil, source) |
|
I've submitted my review on PR #2923 with one suggestion about using the passed context instead of |
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
|
I've already submitted my review on PR #2923. My previous review identified one issue with using The PR is well-structured with good test coverage and follows existing patterns. The suggestion has been provided to the author. No further action needed from me. |
1 participant
Summary
Adds a
cog doctorcommand that diagnoses and auto-fixes common project issues across config, Python code, and environment.--fix): Automatically applies safe fixes to Python source filesChecks included
Motivation
Users upgrading cog versions (especially to 0.17+) hit breaking changes that manifest as confusing build/runtime errors. For example, output classes using
pydantic.BaseModelwitharbitrary_types_allowed=Trueinstead ofcog.BaseModel, or deprecated imports likeExperimentalFeatureWarning. There was no single command to find and fix these.Extensibility
Adding a new check is one file + one line of registration:
pkg/doctor/check_<group>_<name>.goimplementing theCheckinterfaceAllChecks()inregistry.goDesign
Python analysis uses the existing tree-sitter parser (
pkg/schema/python/) — no Python subprocess needed.