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

Core add leg route names #634

Merged

Conversation

binh-dam-ibigroup
Copy link
Collaborator

Requires #632.
This PR centralizes logic to extract a route short/long name from a Leg object.

Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

LGTM. What do you think about adding a function that gets a route label? I think we might already have on in OTP-RR, but might be good to have here instead.
There was a recent PR where I contributed a fix where a function like this would have been useful:
opentripplanner/otp-react-redux@a05c6bd

leg: Pick<Leg, "route" | "routeShortName">
): string => {
const { route, routeShortName } = leg;
if (typeof route === "string") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment to this that explains this block only applies for OTP1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in 1b3839f.

return route?.shortName;
};

/** Extract the route ling name for a leg returned from OTP1 or OTP2. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: ling name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, fixed in 1b3839f.

@binh-dam-ibigroup
Copy link
Collaborator Author

LGTM. What do you think about adding a function that gets a route label?

I found a use of that logic in the fare table, so I will add it. How should that be named? getLegRouteShortOrLongName? getLegRouteName?

@binh-dam-ibigroup
Copy link
Collaborator Author

@daniel-heppner-ibigroup Let me know what you think of 41e4aab with the new function (and revised logic).

Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

Perfect use of that logic. Thank you for the addition, this will be useful in OTP-RR and OTP-UI.

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

This is a great addition thanks so much

@binh-dam-ibigroup binh-dam-ibigroup merged commit 140c1ce into opentripplanner:master Aug 28, 2023
2 checks passed
@binh-dam-ibigroup binh-dam-ibigroup deleted the core-add-leg-route-names branch August 28, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants