Test/estimate coverage #68
No reviewers
Labels
No labels
bug
ci
documentation
duplicate
enhancement
feature
good first issue
help wanted
invalid
performance
priority
P0
priority
P1
priority
P2
question
refactor
size
L
size
M
size
S
size
XL
size
XS
tests
trivial
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
noe.trevino/paso!68
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "test/estimate-coverage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
PR Summary (auto-generated)
@ -0,0 +225,4 @@}```Falls back to `fmt.Stringer`, then `%+v` dump.Line reference is fragile and will break as the file evolves. Consider removing specific line numbers from documentation, or use relative references like "near the top of the file" or anchor to function names instead.
@ -0,0 +332,4 @@```gotype UpdateTaskRequest struct {TaskID intTitle *string // nil means don't updateAnother fragile line reference. Same issue throughout the document - line numbers in documentation create maintenance burden and will become incorrect as code changes.
@ -0,0 +335,4 @@Title *string // nil means don't updateDescription *stringPriorityID *intTypeID *intThe value "138 lines" is metadata that will become stale. Remove file size metrics from documentation.
@ -0,0 +369,4 @@3. **Factory** (`factory.go`, line 53-77):```gofunc NewQuerier(db any, dbType DatabaseType) (Querier, error)Same package testing is mentioned here as a coverage optimization, but this is actually a code smell. You're gaming coverage metrics rather than writing proper integration tests. Tests in the same package can access private implementation details, creating brittle tests that break when refactoring internals that should be hidden.
@ -0,0 +592,4 @@type createHandler struct{}func (h *createHandler) Execute(ctx context.Context, args *Arguments) (any, error) {cliInstance, _ := cli.GetCLIFromContext(ctx)Another fragile line reference (228 lines). This pattern repeats extensively throughout - all these line counts will require constant maintenance.
@ -0,0 +796,4 @@1. **Create command file** (`internal/cli/task/my_command.go`):```gofunc MyCommandCmd() *cobra.Command {The "33.2% coverage" metric is a point-in-time value that will become incorrect. Don't hardcode metrics in documentation.
@ -0,0 +778,4 @@func TestParseAssignArgs(t *testing.T) { /* ... */ }```**Result**: Coverage metrics reflect actual tested code. Went from 0% → 33.2% on CLI task package.Same-package testing is being promoted as the solution here, but this is a testing anti-pattern. If
ParseAssignArgsneeds to be tested, it should be a public function in a service package, not a private utility in a command package. This approach creates fragile tests that couple to implementation details.@ -0,0 +706,4 @@type TaskWriter interface {CreateTask(ctx, req) (*Task, error)UpdateTask(ctx, req) errorDeleteTask(ctx, taskID) errorFragile file count metadata. Don't hardcode "60+ fields" - this will become incorrect.
@ -0,0 +735,4 @@2. **Integration tests** (within service/database packages):- Use in-memory SQLite via `fixtures.SetupTestDB()`- Test service methods with real database operationsMore fragile metadata - "360 lines", "20+ update/view files", etc. All these counts create maintenance burden.
@ -0,0 +219,4 @@Errors: `{"success": false, "error": {"code": "...", "message": "..."}}`3. **Human-readable** (default): Checks if data implements `PrettyPrintable`:```goThe document title says "Paso Architecture" but much of the content is prescriptive how-to guides (see sections like "Adding a New CLI Command"). This creates confusion about the document's purpose. Architectural documentation should describe the system as-is, not teach readers how to extend it. Consider splitting this into separate docs: ARCHITECTURE.md for system design and CONTRIBUTING.md or DEVELOPMENT.md for development guides.
@ -0,0 +353,4 @@---### Database Layer (`internal/database/`)The handler pattern adds abstraction without clear benefit. Compare:
Handler pattern (current):
Direct RunE:
The handler pattern trades 3 lines and a type definition for what? The
Argumentswrapper and auto-formatting could be utility functions. Unless there's a compelling reason (polymorphic command execution, middleware chains), prefer the simpler direct approach.@ -0,0 +399,4 @@### Models (`internal/models/`)**Purpose**: Plain data transfer objects with no database dependencies.The
Argumentsstruct with methods likeMustGetString,GetStringis implementing a string-typed map with getters. This loses compile-time type safety. Why not parse flags directly into a typed struct? The current approach defers validation and makes typos in flag names a runtime error instead of a compile error.@ -0,0 +460,4 @@Databases []DatabaseConfig `yaml:"databases"`ActiveDatabase string `yaml:"active_database,omitempty"`ActiveAssignee string `yaml:"active_assignee,omitempty"`}Exit code 3 for "resource not found" and exit code 4 for "invalid data" are non-standard. Standard practice is:
Custom exit codes beyond this are rarely useful and create confusion. Users checking
$?expect the standard codes. If you need detailed error information, use stderr messages, not exit codes.@ -0,0 +547,4 @@func FormatAssignJSON(result *AssignResult) map[string]any```Command handler becomes thin orchestration:Pointer fields for optional values is correct, but the request struct pattern shown here should be questioned. Consider:
This creates a "bag of fields" that allows nonsensical states. What if all optional fields are nil? That's a no-op update. Better to use the functional options pattern or separate request types for different update operations (UpdateTitle, UpdatePriority, etc.) to make illegal states unrepresentable.
@ -0,0 +857,4 @@}```4. **Register command** (`internal/cli/task/task.go`):The adapter pattern here is over-engineered. You have:
That's 4 layers of indirection to support two databases. The complexity cost is high. Consider: is dual-database support actually needed? If SQLite is for local and PostgreSQL for remote, could you pick one and simplify? The adapter pattern is appropriate when you have many backends or need to swap at runtime, but for two known databases, this might be YAGNI.
@ -0,0 +888,4 @@- **SQLC docs**: https://docs.sqlc.dev/- **Cobra docs**: https://cobra.dev/- **Bubble Tea tutorial**: https://github.com/charmbracelet/bubbleteaFunctional options are being used here, but the example shows only 3-4 options with clear defaults. This pattern shines when you have many options or need to maintain backward compatibility as options grow. For a handful of options, a config struct is simpler:
Direct and explicit. The functional options pattern is valuable for libraries with evolving APIs, but for internal code, it's often overengineering.
@ -0,0 +746,4 @@4. **Acceptance tests**:- `tests/acceptance/cli_validation.sh` - Shell-based CLI validation### Test InfrastructureThe "IoC for Testability" pattern shows extracting pure functions into
*_logic.gofiles to get them tested. This is backward. The problem isn't lack of testability - it's that business logic is in the wrong layer.Instead of:
It should be:
The parsing and validation logic should be in the service layer where it can be tested properly through service tests. The CLI layer should be trivially thin. If you're extracting
*_logic.gofiles from CLI commands, that's a symptom that your service layer is too thin.@ -0,0 +627,4 @@```go// SQLite: returns sqlite.Adapter wrapping generated_sqlite.Queries// PostgreSQL: returns postgres.Adapter wrapping generated_postgres.Queries```These magic numbers (1, 2, 3 for relation types; 1, 2 for task types; 1-5 for priorities) should not be constants. Using integers for categories is a premature optimization that makes the database harder to query and understand.
SELECT * FROM tasks WHERE type = 'feature'is far more maintainable thanWHERE type = 2.If you need numeric IDs for performance, use a lookup table with foreign keys, not hardcoded constants in Go code.
@ -0,0 +277,4 @@**`App` struct** (line 19-35):```gotype App struct {The request lifecycle diagram shows the happy path clearly, but architecture documentation should emphasize failure modes:
The happy path is easy; the failure paths define the architecture.
Review Summary
This PR adds comprehensive architecture documentation (895 lines) while removing obsolete test files and scripts. The documentation has several significant issues:
Documentation Quality Issues
1. Fragile References Throughout
2. Document Purpose Confusion
3. Missing Critical Architecture Details
Architectural Pattern Concerns
1. Testing Anti-Pattern
The document promotes same-package testing (e.g., docs/ARCHITECTURE.md:372, 781, 990) as a solution to get coverage on CLI code. This reveals a deeper problem: business logic is in the wrong layer. If you need to test private functions in command packages, it means the service layer is too thin.
2. Over-Engineering
3. Type Safety Issues
4. Non-Standard Patterns
5. Incorrect Interface Segregation
The example at docs/ARCHITECTURE.md:914 isn't true interface segregation - it's just organizing methods. Segregation requires clients to depend only on methods they use, not just splitting interfaces that are still composed back together.
Recommendations
The deleted files (test files, scripts, example config) are fine to remove.