Add Agency.brandingUrl to GTFS GraphQL #6137
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6137 +/- ##
==========================================
Coverage 69.86% 69.86%
+ Complexity 17495 17494 -1
==========================================
Files 1976 1976
Lines 74711 74712 +1
Branches 7643 7643
==========================================
+ Hits 52195 52196 +1
Misses 19866 19866
Partials 2650 2650 ☔ View full report in Codecov by Sentry. |
leonardehrenfried
changed the title
| @@ -1,9 +1,11 @@ | |||
| //THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
| package org.opentripplanner.apis.gtfs.generated; | |||
|
|
|||
| import graphql.relay.Connection; | |||
There was a problem hiding this comment.
Please remove unused imports (I know it's annoying).
| @@ -1,6 +1,7 @@ | |||
| // THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
| package org.opentripplanner.apis.gtfs.generated; | |||
|
|
|||
| import java.util.HashMap; | |||
| @@ -88,6 +88,8 @@ type Agency implements Node { | |||
| """ | |||
| types: [AgencyAlertType] | |||
| ): [Alert] | |||
| "URL to the logo of the agency" | |||
| brandingUrl: String | |||
There was a problem hiding this comment.
I know that I has been imported for quite a while but I cannot actually find this in the GTFS spec. Can you point me towards it?
If it's not in the specification, we would not like to expose in the API and instead remove it from the data model.
If you're keen to use it, can you open a PR or issue on the GTFS spec repo?
There was a problem hiding this comment.
No, I didn't find it in the GTFS spec but in the onebusaway model as optional so I thought it could be added also in the Graphql schema also as optional.
There was a problem hiding this comment.
So there is no way to add brandingUrl aside from opening a Issue or PR to the GTFS spec repo?
There was a problem hiding this comment.
We really would like to not expose anything that isn't in the spec. If you want to discuss this, you can come to the dev meeting.
There was a problem hiding this comment.
It was a mistake to have it in OBA and OTP in the first place.
There was a problem hiding this comment.
Ok, thanks. I'll mark this PR as closed then
Summary
Add Agency.brandingUrl property in the schema.graphql and it's implementation
Unit tests
Write a few words on how the new code is tested.
pass the continuous integration service
? Yes
Documentation
I added a proper doc comment in the graphql schema