Repository navigation
refactor: unify GTFS parsing and real-time fetching across coverage and vehicle checks - #72
Conversation
|
Follow up
|
…ckBundleExpiration
234e320 to
3d357f9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors GTFS parsing and real-time feed fetching in metric checks to use centralized helper functions for consistency.
- Replaced manual GTFS-RT HTTP request logic in
CountVehiclePositionswithFetchGTFSRTFeed. - Unified static GTFS parsing in
CheckBundleExpirationandCheckAgenciesWithCoverageviaParseGTFSFromCache. - Updated imports accordingly and added unit tests for
FetchGTFSRTFeed.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/metrics/gtfs-realtime-bindings.go | Swapped manual HTTP fetch for unified FetchGTFSRTFeed call. |
| internal/metrics/bundle_expiration.go | Replaced direct file reading and ParseStatic with ParseGTFSFromCache. |
| internal/metrics/agencies_with_coverage.go | Replaced direct file reading and ParseStatic with ParseGTFSFromCache and streamlined error return. |
| internal/gtfs/gtfs_bundles_test.go | Added tests for FetchGTFSRTFeed, covering success and failure cases. |
Comments suppressed due to low confidence (2)
internal/metrics/gtfs-realtime-bindings.go:21
- This new call to
FetchGTFSRTFeedinCountVehiclePositionsisn't covered by existing tests; consider adding a unit test forCountVehiclePositionsto validate correct feed retrieval and error handling.
resp, err := gtfs.FetchGTFSRTFeed(server)
internal/metrics/bundle_expiration.go:18
- The new call to
ParseGTFSFromCacheinCheckBundleExpirationisn't covered by any tests; add tests to verify successful parsing and error scenarios.
staticData, err := gtfs.ParseGTFSFromCache(cachePath, server.ID)
| "vehicle_position_url": server.VehiclePositionUrl, | ||
| }, | ||
| }) | ||
| return 0, err |
There was a problem hiding this comment.
The error returned by FetchGTFSRTFeed is not wrapped or reported with additional context; consider using fmt.Errorf and/or reporting it via report.ReportErrorWithSentryOptions to preserve diagnostic information.
| return 0, err | |
| wrappedErr := fmt.Errorf("failed to fetch GTFS-RT feed from server %d (%s): %v", server.ID, server.VehiclePositionUrl, err) | |
| report.ReportErrorWithSentryOptions(wrappedErr, report.SentryReportOptions{ | |
| Tags: map[string]string{ | |
| "server_id": strconv.Itoa(server.ID), | |
| "vehicle_position_url": server.VehiclePositionUrl, | |
| }, | |
| }) | |
| return 0, wrappedErr |
There was a problem hiding this comment.
The error returned by
FetchGTFSRTFeedis not wrapped or reported with additional context; consider usingfmt.Errorfand/or reporting it viareport.ReportErrorWithSentryOptionsto preserve diagnostic information.
The error is already reported to Sentry inside the FetchGTFSRTFeed function.
| ) | ||
|
|
||
| // CheckBundleExpiration calculates the number of days remaining until the GTFS bundle expires. | ||
| func CheckBundleExpiration(cachePath string, logger *slog.Logger, currentTime time.Time, server models.ObaServer) (int, int, error) { |
There was a problem hiding this comment.
Both logger and currentTime parameters appear unused in the updated implementation; consider removing them or adding appropriate logging/time-based logic.
| func CheckBundleExpiration(cachePath string, logger *slog.Logger, currentTime time.Time, server models.ObaServer) (int, int, error) { | |
| func CheckBundleExpiration(cachePath string, server models.ObaServer) (int, int, error) { |
There was a problem hiding this comment.
Both
loggerandcurrentTimeparameters appear unused in the updated implementation; consider removing them or adding appropriate logging/time-based logic.
I removed the logger, but since the current time is used inside the function, I kept it.
…iesWithCoverage - Removed unnecessary logger argument - Updated all callers accordingly to use centralized logging
|
I have made Copilot suggestions. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
looks good, great work!
CheckAgenciesWithCoverageandCheckBundleExpirationto useParseGTFSFromCachefor consistent static GTFS parsing.CountVehiclePositionsto useFetchGTFSRTFeedfor standardized real-time feed retrieval.