Prevent an infinite loop when sorting trips #427
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #427 +/- ##
========================================
+ Coverage 5.3% 5.39% +0.09%
========================================
Files 322 322
Lines 15469 15532 +63
Branches 4674 4695 +21
========================================
+ Hits 820 838 +18
- Misses 12497 12521 +24
- Partials 2152 2173 +21
Continue to review full report at Codecov.
|
|
@evansiroky, can you describe how this scenario (editing trips with no stop times) came about? I agree that we should not fall into an infinite loop, but I'm a bit confused about the larger context. Ideally, we would prevent users from editing trips with no stop times, but I guess if a feed were imported with no stop times, this situation could occur. I'll also note that just because we have issue and PR templates with a bunch of sections to fill out now, we should be careful to still create well-crafted tickets. I think the templates can quickly become a nuisance that get ignored especially if they don't apply to every situation well. |
|
Agreed that I should have filled out issue #426 better. I just edited that issue to wrote about how I discovered the bug while changing the service_id and then looking at the timetable editor. |
| tripId: string, | ||
| useFrequency: boolean | ||
| tripShortName: ?string, | ||
| useFrequency?: boolean, |
There was a problem hiding this comment.
Is this type change correct? Shouldn't useFrequency always be defined because it is defined in the GraphQL query?
There was a problem hiding this comment.
I believe if the value is undefined in the database, it will not come through in the response of the GraphQL query. If you look at a response from GraphQL in chrome dev tools you should see this.
| } | ||
| if (a.stopTimes.length === 0 && b.stopTimes.length === 0) return 0 | ||
| if (a.stopTimes.length === 0) return -1 | ||
| if (b.stopTimes.length === 0) return 1 |
There was a problem hiding this comment.
This code block definitely deserves a descriptive comment describing: why it's sorting this way, why there might be a trip with no stop times, etc. It's the only change to non-test code in the whole PR, I think it's important that something like this have some comments.
|
Just added some more comments about the additional checks in the sort function. |
|
🎉 This PR is included in version 4.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Checklist
devbefore they can be merged tomaster)Description
This PR adds some tests to make sure that infinite loops don't happen when trips have an empty array of stop times. Fixes #426.