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

Slight refactoring of NTT code to prep for additional v3 changes #69

Merged
merged 4 commits into from
Dec 3, 2021

Conversation

nathan-weinberg
Copy link
Member

Minor changes made:

  • LICENSE and README updates
  • Added a bunch of comments
  • Renamed a few functions with references to cars changed to vehicles since Silver Line buses are tracked now as well
  • Renamed data function to trains since it fetches train data

Less minor changes made:

  • Retooled some of the logic in vehicle_data_for_routes() and vehicle_array_is_new()

The idea behind these changes was to preserve the test_mode functionality while ensuring that data for all trains in the tracker was accurate - previous logic defined all trains as new when test_mode was active - current logic should still include all trains in test_mode but their JSON data should accurately show whether or not they are new

  • Removed references to newOnly in getTrainRoutePairsForLine()

Not going to lie, I only did this b/c it was the only way to get test mode to work with the changes I made described above. I am not sure why this was needed. Please double check this.

I did test these changes and trains are tracking properly, BUT I am not sure how this will affect interactions with the database, SO we should definitely consider that before merging this PR.

Copy link
Contributor

@austinjpaul austinjpaul left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this cleanup!
A couple things caught my eye, mainly in mbta_api.py. Of course, these just reflect my opinions and i'm happy to be overridden on any of them.

server/fleet.py Show resolved Hide resolved
server/mbta_api.py Outdated Show resolved Hide resolved
server/mbta_api.py Outdated Show resolved Hide resolved
server/mbta_api.py Outdated Show resolved Hide resolved
server/mbta_api.py Outdated Show resolved Hide resolved
Copy link
Contributor

@idreyn idreyn left a comment

Choose a reason for hiding this comment

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

Looks good! Very sorry it took me this long to review. Thanks for taking this on!

server/fleet.py Show resolved Hide resolved
server/mbta_api.py Outdated Show resolved Hide resolved
server/mbta_api.py Outdated Show resolved Hide resolved
src/useMbtaApi.js Outdated Show resolved Hide resolved
@nathan-weinberg nathan-weinberg merged commit 5fa9a04 into transitmatters:master Dec 3, 2021
@nathan-weinberg nathan-weinberg deleted the refactor branch December 3, 2021 03:05
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