-
Notifications
You must be signed in to change notification settings - Fork 34
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
SilverRail: MODE_TRAIN schema #107
base: master
Are you sure you want to change the base?
Conversation
"description": "Seat number, e.g. 42", | ||
"type": "string", | ||
"minLength": 1 | ||
}, |
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.
Could we merge the 'coach' and 'seatNumber' to just 'seat' or something that would uniquely identify the place you have. Remember that from MaaS-backend viewpoint, we only need to identify the place, whereas TSP can parse whatever it offered.
That might make it more compatible with providers that could not make it as unique (cross-compare e.g. with VR). BTW VR also identifies the number of 'car' (vaunu) (or is that what coach denotes here?), which might help identifying these places.
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.
Yes we could. I'd keep them separate to allow for flexibility on the UI side - "coach" is the same as "car" ("The coach in which the seat is located." according to the SR docs).
schemas/core/booking.json
Outdated
"minLength": 1 | ||
} | ||
}, | ||
"additionalProperties": false |
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.
Remember to set '"required": ["trainId", ...] if you intend to expect & use these variables - otherwise the caller may leave them away.
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.
Ah, left it out since I didn't have the trainId
in the options-list
response but luckily the info is available at that stage - made a commit to improve booking-silverrail-options-list so that tests will pass with the trainId
set as required.
schemas/core/units.json
Outdated
@@ -180,7 +180,7 @@ | |||
"currency": { | |||
"description": "Accepted monetary unit in ISO 4127 format, see https://en.wikipedia.org/wiki/ISO_4217#cite_note-1", | |||
"type": "string", | |||
"enum": [ "EUR" ] | |||
"enum": [ "EUR", "GBP" ] |
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.
Could we get the unit change as a separate PR? That would help merging it together with the actual implementation changes.
BTW It might be that we need external meta somewhere to denote what are the actual currencies the particular TSP accepts. I believe SilverRail only operates with pounds?
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.
I'll separate this to another PR.
The current configuration that we have uses pounds but it might also be possible to switch it to EUR - but that's uncertain.
Moved the GBP related stuff to #109 |
Notes: