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

Add v3 utils files in order to remove Route Summary #311

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

alanna-zhou
Copy link
Contributor

Overview

For a v3, iOS let me know that they don't need Route Summary anymore, and Android kindly is (in the process of?) pivoting to deprecating Route Summary! The point of this PR is to make v3 super clean and significantly less bulkier than v1 and v2. This may seem contradictory rn because I have to add extra files, but it's the only way I can make changes so that we can still offer backwards compat. Maybe a couple years down the line the next backend member can refactor these files into v1, but for now we're just going to have to deal with multiple versions of utils files :')

Changes Made

Add ParseRouteUtilsV3 and a RouteUtilsV3 so that we can remove Route Summary as a field for a route in v3/route.

Test Coverage

Tested locally and it successfully removes route summary :)
I'll deploy this onto the devserver and see if it breaks integration before merging into master

Next Steps

Get rid of routeID and analytics

Related PRs or Issues

#307

Copy link
Member

@chalo2000 chalo2000 left a comment

Choose a reason for hiding this comment

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

when parseRoutes has >200 lines 😱 goodluck transit

src/utils/ParseRouteUtilsV3.js Outdated Show resolved Hide resolved
*/
if (previousDirection.routeNumber === direction.routeNumber) {
route.directions.splice(index - 1, 2, mergeDirections(previousDirection, direction));
index -= 1; // since we removed an element from the array
Copy link
Member

Choose a reason for hiding this comment

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

put this before route.directions.splice so you don't have to subtract twice (efficiency 🤩 transit will so much faster now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yikes im worried about making too many changes, so i'm going to hold off on this (since i didn't write any of this)

src/utils/ParseRouteUtilsV3.js Outdated Show resolved Hide resolved
src/utils/ParseRouteUtilsV3.js Show resolved Hide resolved
}

/**
* Returns whether [route] contains a bus trasnfer
Copy link
Member

Choose a reason for hiding this comment

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

transfer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe tyty for all these helpful comments lol i didnt even bother fixing the old stuff xD

@ckdesir
Copy link
Member

ckdesir commented Oct 27, 2020

transit should be good w/o routeSummary. i see that routeNumber was removed via #308 but that should be a simple fix!

@alanna-zhou
Copy link
Contributor Author

transit should be good w/o routeSummary. i see that routeNumber was removed via #308 but that should be a simple fix!

thanks chris!! i haven't removed routeNumber yet -- that's gonna be my next PR :)

@alanna-zhou alanna-zhou merged commit 1aa9fd4 into master Oct 27, 2020
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