fix: support duplicate reporter output paths#481
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the validator’s reporter configuration to allow specifying the same reporter type multiple times (e.g., multiple JSON outputs), and adds an integration-style test case to verify it.
Changes:
- Replace reporter configuration map with a slice-based
reporterConfigto support duplicates and preserve ordering. - Update reporter flag parsing, validation, and reporter construction to use the new representation.
- Add a txtar test demonstrating duplicate reporter types writing to separate files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/validator/validator.go | Switch reporter config from map to slice (reporterConfig) and update parsing/validation/building to support duplicate reporters. |
| cmd/validator/testdata/reporters.txtar | Add coverage for passing the same reporter type multiple times with different output files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts := strings.Split(reportFlag, ":") | ||
| switch len(parts) { | ||
| case 1: | ||
| conf[parts[0]] = "" | ||
| conf = append(conf, reporterConfig{reportType: parts[0]}) | ||
| case 2: | ||
| if parts[1] == "-" { | ||
| conf[parts[0]] = "" | ||
| conf = append(conf, reporterConfig{reportType: parts[0]}) | ||
| } else { | ||
| conf[parts[0]] = parts[1] | ||
| conf = append(conf, reporterConfig{reportType: parts[0], outputDest: parts[1]}) | ||
| } | ||
| default: | ||
| return nil, errors.New("wrong parameter value format for reporter, expected format is `report_type:optional_file_path`") |
| func buildReporters(reportType []reporterConfig) ([]reporter.Reporter, error) { | ||
| reporters := make([]reporter.Reporter, 0, len(reportType)) | ||
| for rt, of := range reportType { | ||
| reporters = append(reporters, getReporter(rt, of)) | ||
| for _, rc := range reportType { | ||
| reporters = append(reporters, getReporter(rc.reportType, rc.outputDest)) | ||
| } | ||
| return reporters, nil | ||
| } |
macayu17
force-pushed
the
fix/480-duplicate-reporters
branch
from
bea84a8 to
2284cd7
Compare
kehoecj
added
OSS Community Contribution
kehoecj
left a comment
kehoecj
left a comment
There was a problem hiding this comment.
@macayu17 PR looks solid and passed my local stress testing! I did a big docs refactor ahead of your PR. You'll need to rebase but it shouldn't be a big deal because it was docs only. A few changes you'll need to make:
- Need a CHANGELOG entry
- Update the docs with this new feature, probably just here
- Add a test for duplicate reporters writing to the same file. What happens with
--reporter=json:same.json --reporter=json:same.json?
kehoecj
added
pr-action-requested
macayu17
force-pushed
the
fix/480-duplicate-reporters
branch
from
2284cd7 to
196951c
Compare
|
Rebased on I added the changelog entry, documented repeating the same reporter type with different output paths, and added coverage for duplicate JSON reporters writing to the same file. For the same-file case, both reporters run successfully and the final file remains a single valid JSON report at that path. |
kehoecj
removed
the
pr-action-requested
Fixes #480
Summary
--reporterflag instead of keying reporter config by type.Test
go test ./cmd/validator -run "Test_getFlags|Test_getFlagsValues|Test_emptyBoolEnvVarNoParseError|TestScript/ reporters" -count=1 -vgo vet ./cmd/validatorgofmt -s -l -e cmd\validator\validator.gogit diff --check