Skip to content
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

WIP Next return Journey feature #19

Open
wants to merge 2 commits into
base: WIP-operator-complete-api-initial-version
Choose a base branch
from

Conversation

osarrat
Copy link
Collaborator

@osarrat osarrat commented Apr 26, 2022

Based on our discussion on how to manage return journey, a brief PR to manage this dimension of the standard.

The aim of the feature is to retrieve the next return journey with the same driver, if any, as supported by RDEX+.

The objectives of this PR are two-fold:

  1. provide an optional feature to present in the search result the "next return journey" : this return journey is a secondary but interesting parameter to decide which outward journey to take
  2. backward compatibility with RDEX requires to cover this need of "return journey"

@ccyrille
Copy link

@osarrat following our discussion I added the /journeys endpoint

description: Ok. Request processed successfully.
content:
application/json:
schema:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint takes several ids and returns a single journey - I believe you meant to return an array here, right?

operationId: getJourneys
parameters:
- in: query
name: ids[]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should put the brackets in the name, as we have not at other places.

@riclage
Copy link

riclage commented Jun 8, 2022

@adelcasse @osarrat We are discussing today whether this PR is really needed. I know this has been discussed before but could we reconsider the need for the property nextReturnJourneyId? At first having it as optional seemed simple enough but now it is also requiring a completely new endpoint, the /journeys one.

My concern is that we are trying to make edge cases fit into the standard and the risk is of complexifying the spec for little gain. With that in mind, could we, for example, reconsider the idea of simply calling the search again with the start/end addresses reversed to find potential return journeys?

@osarrat
Copy link
Collaborator Author

osarrat commented Jun 14, 2022

Hi @riclage ,
I understand your concern. It is hard to find a balance between two objectives of the standard : simplicity, and backward compatibility with both Carpool IDFM and RDEX.
By the way, as far as I remember, the new GET /journey endpoint was also designed in combination with the GET regular_trips routes in order to retrieve a specific journey par of a regular trip.
But I'm open to discuss it again during tomorrow's meeting. Let see if we can find together a better tradeoff ! :)

@pierrecamilleri
Copy link
Collaborator

Following today's discussion, we decided to keep this PR unmerged, but ready if the need for this functionality would arise in the future.

In more details :

  • Calling an additional search with reversed origin/destination does not cover exactly/easily this use case, as it is expected to find a return trip with the same driver (I updated the description for clarity), which may happen a different day for instance.
  • We agree that the current PR is a reasonable solution to implement this functionality.
  • However, nobody in the working group is actually using this functionality, so for the sake of keeping the standard as simple as possible, we decided to wait until the need arises in practice before merging it.
  • The subject of functional retro-compatibility needs to be further discussed (is it OK not to have 100% functional retrocompatibility?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants