OVS 3.0: SD-2234: Add WaterWays to schedules - #651
Conversation
PR Summary by QodoAdd waterway locations to OVS 3.0.3 schedules
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
🟡 Changes recommended
The parameters: field for /v3/service-schedules is currently indented into the description: | block, which would break the OpenAPI structure for that operation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the OVS 3.0.3 OpenAPI specification to introduce waterway (canals/straits) support in vessel schedules by extending TransportCall.location with a WWAY location type and documenting the expected semantics for timestamps and related fields.
Changes:
- Add
WaterwaySMDGLocationschema and wire it intoTransportCall.locationvia discriminator mapping andoneOf. - Update schedule and filter documentation to describe
TransportCallas representing either ports/terminals or waterways/entry points. - Clarify timestamp and
facilityTypeCodesemantics forWWAYtransport calls.
File summaries
| File | Description |
|---|---|
| ovs/v3/OVS_v3.0.3.yaml | Extends the OVS v3.0.3 spec with WWAY location support and updates related descriptions/filters. |
Review details
Suppressed comments (2)
ovs/v3/OVS_v3.0.3.yaml:63
- Grammar issue: "specify mutually filters" is ungrammatical; it should describe that the filters are mutually exclusive.
Be aware that it is possible to specify mutually filters, resulting in an empty response list. For example:
ovs/v3/OVS_v3.0.3.yaml:71
- Minor doc typos/grammar: "request data" should be "request date", and "endDate filters is" should be "endDate filter is".
If no `startDate` filter is provided, **3 months** before the request data is used. If no `endDate` filters is provided, **6 months** after the request date is used.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b44d12d |
There was a problem hiding this comment.
🟡 Changes recommended
The updated documentation/spec text contains a release-date placeholder and several ambiguous phrasings that could mislead API consumers about how WWAY is represented in the schema.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
ovs/v3/OVS_v3.0.3.yaml:764
- In the
eventTypeCodedescription, "WWAYTransportCall" is ambiguous becauseWWAYis alocation.locationTypediscriminator. Clarify that the semantics apply when the containing TransportCall haslocation.locationType = WWAY(and likewise for the "other end" example).
For a `WWAY` **TransportCall**, `ARRI` means arrival at the waterway location represented by that **TransportCall**, and `DEPA` means departure from the same location.
When `waterwaySMDGEntryPointCode` is provided, both event types refer to that specific waterway point. `DEPA` does not mean that the vessel has exited the complete waterway at the point on the opposite side.
A carrier may publish the point at the other end as another ordered `WWAY` TransportCall.
ovs/v3/OVS_v3.0.3.yaml:835
- Same ambiguity here: "WWAY TransportCall" should be phrased in terms of
location.locationTypeto match the actual schema structure and reduce misinterpretation.
The `BRTH` default is retained for backwards compatibility. If schema-driven tooling materializes `BRTH` for a WWAY TransportCall, consumers **MUST** disregard the value and **MUST NOT** interpret it as vessel berthing.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b249f70 |
There was a problem hiding this comment.
🟡 Changes recommended
The spec text currently references a non-existent VesselSchedules name, which should be corrected to match the actual schema/property names before publishing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
ovs/v3/README.md:7
- The changelog entries in this README typically include a release date in parentheses (e.g., v3.0.2, v3.0.1). v3.0.3 is missing a date, which makes it harder to correlate the spec with a release timeline.
<a name="v303"></a>[Release v3.0.3](https://app.swaggerhub.com/apis-docs/dcsaorg/DCSA_OVS/3.0.3)
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit a376435 |
There was a problem hiding this comment.
🔵 Needs a closer look
There is at least one concrete documentation/spec issue (undefined type name in the endpoint description) that should be corrected to avoid confusing API consumers.
Review details
Suppressed comments (2)
ovs/v3/OVS_v3.0.3.yaml:43
- In this description,
VesselSchedulesis referenced as a schema/type name, but the schema defined incomponents/schemasisVesselSchedule(singular). Using an undefined type name in the API description can confuse consumers and generated docs.
The response is an array of `ServiceSchedule` objects. Each `ServiceSchedule` can contain one or more `VesselSchedules`.
ovs/v3/README.md:7
- Release headings in this changelog typically include the release date (e.g., v3.0.2 and v3.0.1). Adding the v3.0.3 release date would keep the changelog consistent and make it easier to track when the patch was published.
<a name="v303"></a>[Release v3.0.3](https://app.swaggerhub.com/apis-docs/dcsaorg/DCSA_OVS/3.0.3)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit de4a6b5 |
SD-2234: Implementation of waterways (canals & straits)