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

fix: Use IDs for Stoughton routes on Providence/Stoughton line for Fall 2023 diversion #671

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

clem-hertling
Copy link
Contributor

Summary of changes

Asana Ticket: 🚧 Splice together new Stoughton Branch shapes

@clem-hertling clem-hertling requested a review from jfabi September 14, 2023 19:38
@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch 2 times, most recently from 44571aa to 0878ace Compare September 14, 2023 21:53
Copy link
Member

@jfabi jfabi left a comment

Choose a reason for hiding this comment

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

I'll be honest, I didn't quite understand whether or not it would be expected for this to be needed with the new, non-typical pattern, at first...but upon closer inspection, yes, this looks right to me.

A few additional things, however:

  • In apps/state/config/config.exs, please add the Fairmount Line-specific stations to the not_on_route configuration for the Providence/Stoughton Line; you'll see that this has already been done for the Franklin/Foxboro Line. This is sort of an insurance that will make sure we don't accidentally associate those Fairmount Line stations with the Providence/Stoughton Line on MBTA.com pages...
  • Over in https://github.com/mbta/gtfs_creator/pull/2125, I'm going to suggest that we add "via" messaging to the route_pattern_names, and that might result in changes to the names here... I would suggest making those changes first and then ensuring that the API validation still passes on that branch.
  • AFAIK (unless you've heard otherwise), this change to add the new pattern is permanent, so you should remove the "temporarily" wording from the PR and commit titles.

@clem-hertling
Copy link
Contributor Author

mmh okay so,

  • fair enough, I'll add this!
  • did that, and it does change the name, which breaks the validation, so I'll fix that here now
  • I thought the new pattern was only being used during work on the tracks? Or is that the one that's related to the Amtrak changes? either way, I can remove the temporarily, yeah

@clem-hertling
Copy link
Contributor Author

though after checking, you're right: with the new non-typical patterns, this doesn't look like it's needed... but are the fairmount patterns really non-typical?

@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch from 0878ace to d357c24 Compare September 18, 2023 14:59
@github-actions
Copy link

Coverage of commit d357c24

Summary coverage rate:
  lines......: 89.6% (4046 of 4515 lines)
  functions..: 70.8% (2217 of 3133 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch from d357c24 to c89db41 Compare September 18, 2023 15:11
@clem-hertling clem-hertling changed the title fix: Temporarily allow 3 routes on Providence/Stoughton line for Fall 2023 diversion fix: Use IDs for Stoughton routes on Providence/Stoughton line for Fall 2023 diversion Sep 18, 2023
@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch 3 times, most recently from 6c0c808 to 81ed7c8 Compare September 18, 2023 16:29
@github-actions
Copy link

Coverage of commit 81ed7c8

Summary coverage rate:
  lines......: 89.6% (4046 of 4515 lines)
  functions..: 70.8% (2217 of 3133 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@clem-hertling
Copy link
Contributor Author

ah, nevermind, I get it now. whoops!

@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch 2 times, most recently from f7f2c2c to 2c1668f Compare September 18, 2023 17:26
@clem-hertling
Copy link
Contributor Author

I can't merge this PR bc the test failed, but I think that's because it tests on gtfs_creator's master branch, so I can't really solve this without breaking gtfs_creator beforehand

@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch from 2c1668f to 437c642 Compare September 20, 2023 19:15
@clem-hertling clem-hertling force-pushed the ch-fall23-stoughton-branch branch from 437c642 to 2e93668 Compare September 20, 2023 19:24
@github-actions
Copy link

Coverage of commit 2e93668

Summary coverage rate:
  lines......: 89.6% (4046 of 4515 lines)
  functions..: 70.8% (2217 of 3133 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@clem-hertling clem-hertling merged commit bb6baf3 into master Sep 20, 2023
7 checks passed
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.

2 participants