-
Notifications
You must be signed in to change notification settings - Fork 8
validator command can now output JSON to stdout, with extras optionally included #537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Generated Code Changes RequiredThe following files may need to be regenerated. Please run diff --git a/doc/cli/transitland_validate.md b/doc/cli/transitland_validate.md
index e92dade..082ca72 100644
--- a/doc/cli/transitland_validate.md
+++ b/doc/cli/transitland_validate.md
@@ -16,21 +16,26 @@ transitland validate [flags] <reader>
% transitland validate "https://www.bart.gov/dev/schedules/google_transit.zip"
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds JSON output functionality to the validator command, allowing validation results to be written to stdout in JSON format with log suppression, making it easier to programmatically consume the output. The changes also expose options to include additional data in the JSON output such as GTFS entities, route geometries, and service levels.
Key Changes:
- Added
--output-format jsonflag to output JSON to stdout while suppressing log messages - Added three new include flags (
--include-entities,--include-route-geometries,--include-service-levels) to optionally include extra data in JSON output - Renamed the output flag from
-oto--output/-owith improved help text
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Validate output format | ||
| if cmd.OutputFormat != "" && cmd.OutputFormat != "json" { | ||
| return errors.New("invalid output format: only 'json' is supported") |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message states "only 'json' is supported" but this might be confusing since the empty string is also valid (representing the default behavior). Consider clarifying the error message to something like "invalid output format: must be empty or 'json'" or "invalid output format: supported values are 'json' or empty for default".
| return errors.New("invalid output format: only 'json' is supported") | |
| return errors.New("invalid output format: supported values are 'json' or empty for default") |
| // Set log level to fatal when outputting JSON to stdout to prevent log pollution | ||
| if !cmd.shouldShowLogs() { | ||
| log.SetLevel(zerolog.FatalLevel) | ||
| } |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the global log level in Parse could affect other loggers if multiple commands are run in the same process or if this code is used as a library. Consider using a context-aware logger or storing the log level preference in the command struct and checking it before each log call, similar to how shouldShowLogs is already being checked at lines 105 and 150.
| // Set log level to fatal when outputting JSON to stdout to prevent log pollution | |
| if !cmd.shouldShowLogs() { | |
| log.SetLevel(zerolog.FatalLevel) | |
| } | |
| // Do not set global log level; rely on shouldShowLogs() checks before logging |
| outf := os.Stdout | ||
| if cmd.OutputFile != "" { | ||
| var err error | ||
| outf, err = os.Create(cmd.OutputFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer outf.Close() | ||
| } |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When both OutputFile and OutputFormat are set to json, the JSON output goes only to the file and not to stdout. This might be unexpected behavior - users might expect JSON to be written to both locations, or at least have logs explaining where the output went. Consider documenting this behavior in the help text or adding a validation check to prevent both flags from being used together.
| fl.BoolVar(&cmd.Options.IncludeRealtimeJson, "rt-json", false, "Include GTFS-RT proto messages as JSON in validation report (default: false)") | ||
| fl.BoolVar(&cmd.SaveValidationReport, "validation-report", false, "Save static validation report in database (default: false)") | ||
| fl.StringVar(&cmd.ValidationReportStorage, "validation-report-storage", "", "Storage path for saving validation report JSON") | ||
| fl.IntVar(&cmd.FVID, "save-fvid", 0, "Save report to feed version ID") | ||
| fl.StringSliceVar(&cmd.rtFiles, "rt", nil, "Include GTFS-RT proto message in validation report") | ||
| fl.IntVar(&cmd.Options.ErrorLimit, "error-limit", 1000, "Max number of detailed errors per error group") | ||
| fl.StringSliceVar(&cmd.rtFiles, "rt", nil, "Include GTFS-RT proto message in validation report (default: none)") | ||
| fl.IntVar(&cmd.Options.ErrorLimit, "error-limit", 1000, "Max number of detailed errors per error group (default: 1000)") |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding explicit default values in the help text like "(default: false)" and "(default: 1000)" is inconsistent with the codebase convention. The convention in this codebase is to only document defaults in help text when they come from environment variables (e.g., "default: $TL_DATABASE_URL"). The pflag library already displays the default values automatically, so these additions are redundant. Consider removing the explicit default value documentation from lines 61, 62, 65, and 66.
| func (cmd *ValidatorCommand) AddFlags(fl *pflag.FlagSet) { | ||
| fl.StringSliceVar(&cmd.extensionDefs, "ext", nil, "Include GTFS Extension") | ||
| fl.StringVar(&cmd.OutputFile, "o", "", "Write validation report as JSON to file") | ||
| fl.StringVarP(&cmd.OutputFile, "output", "o", "", "Write validation report as JSON to file") |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The long flag name "output" is inconsistent with the similar RTConvertCommand which uses "out" for its output file flag. For consistency across the codebase, consider using "out" as the long flag name instead of "output".
| fl.StringVarP(&cmd.OutputFile, "output", "o", "", "Write validation report as JSON to file") | |
| fl.StringVarP(&cmd.OutputFile, "out", "o", "", "Write validation report as JSON to file") |
No description provided.