Make route comparator tolerate all kinds of data #124

evansiroky · 2020-01-28T19:57:56Z

Adds a few functions to determine validity of various route values.
Refactors route sort order function to sort using various different data formats for routes
Adds a test suite to check for all kinds of possible route data being sorted properly

landonreed · 2020-01-28T22:29:55Z

lib/util/itinerary.js

+ *    cannot be parsed as integers will be placed beneath those that are valid.
+ * 3. In case of the same routeShortName criteria, attempt to sort based off of
+ *    the string routeShortName or routeLongName if the routeShortName is
+ *    undefined.


I'm confused... this JSdoc suggests that it checks the route sort order and then uses route short names to further sort (and so on for short name/long name). Where in the code is that actually handled? It appears that once a valid routeSortOrder comparison is made, the function simply returns.

Bah, you're right.

Dang, you really had me scratching my head!

codecov-io · 2020-01-29T06:18:42Z

Codecov Report

Merging #124 into fix-route-order will increase coverage by 0.7%.
The diff coverage is 77.33%.

@@                Coverage Diff                 @@
##           fix-route-order     #124     +/-   ##
==================================================
+ Coverage            10.65%   11.35%   +0.7%     
==================================================
  Files                  133      133             
  Lines                 6090     6154     +64     
  Branches              1758     1781     +23     
==================================================
+ Hits                   649      699     +50     
- Misses                4618     4634     +16     
+ Partials               823      821      -2

Impacted Files	Coverage Δ
lib/components/viewers/route-viewer.js	`0% <0%> (ø)`	⬆️
lib/util/itinerary.js	`28.4% <80.55%> (+11.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b940040...f9f208d. Read the comment docs.

evansiroky · 2020-01-29T06:20:01Z

All right, this one should actually work.

__tests__/util/itinerary.js

lib/util/itinerary.js

landonreed · 2020-01-29T15:47:18Z

lib/util/itinerary.js

+    // b is a valid number, b gets priority
+    if (isNaN(aVal)) return 1
+    // a is a valid number, a gets priority
+    if (isNaN(bVal)) return -1


I'm a little worried about embedding this priority for numbers over strings in this comparator. If we decide that strings should come before numbers, we would have to refactor this function pretty heavily. There are a number of cases where we may actually want to give strings priority over ints (think about how TriMet lists their routes). Perhaps this function should also except a boolean arg stringsFirst to indicate whether strings should be given higher priority in the sort (and these lines could use that arg)?

I went ahead and added an option to forgo sorting by integer shortName. I think it'd be best to keep that because I can see it being quite common to have routes with "numeric" shortNames (ex: "3", "10", "20") and if we didn't sort by integer shortName, we'd get non-numerically sorted values (ex: "10", "20", "3").

Sorry, I think maybe my initial point wasn't super clear. I do think we should sort shortNames as integers; however, I think that in some cases (maybe all) it might be preferable to have non-integer shortNames (and perhaps longNames) show up before integer shortNames in a list. E.g., Agency X may have numerically-named bus routes and color-named train lines. Because the train lines are more heavy utilized (and likely a shorter list) they might should show up first for better discoverability. So the order we're looking for would be:

BLUE GREEN RED 1 2 10 19 22 25 100 etc.

I don't think what we have here is doing that based on my reading and the snapshots.

I think so too. I'm also in favor of having one snapshot that has all the cases, so it shows what a normal route list could look like.

@landonreed and @binh-dam-ibigroup, I just pushed a new commit that introduces two more comparator functions which I think will address these concerns. I've added a route type comparator that sorts based on route type. This will prioritize fixed guideway transit and ferries over buses. Secondly, I added a comparator that prioritizes routes with shortNames that begin with an alphabetic character.

@landonreed and @evansiroky: Where would this case fit?
{ shortName: "10C" }

landonreed · 2020-01-29T15:48:28Z

lib/util/itinerary.js

+    // a is not a comparable string, b gets priority
+    if (!aVal) return 1
+    // b is not a comparable string, a gets priority
+    if (!bVal) return -1


Again, wondering about this embedded logic similar to https://github.com/opentripplanner/otp-react-redux/pull/124/files#r372464064

landonreed

I was initially wary of how you structured all of this, but it has really grown on me. Nice work!

A couple of comments on how things are structured. Big one is on clarifying the comment for routeComparator and whether we should embed the strings after numbers logic in the make Comparator functions.

rename some functions, use consistent arguments and allow disabling integer route short name sorting

evansiroky · 2020-01-30T08:03:54Z

See additional changes and this comment: #124 (comment)

I initially thought OTP-RR #124 was going to be a simple fix, hence the quick review; now it looks it like we are rewriting the route sorting entirely.

__tests__/util/itinerary.js

…rtName criteria

landonreed · 2020-02-04T17:56:26Z

__tests__/util/__mocks__/itinerary.json

+    "shortName": "Yellow",
+    "sortOrder": 2
+  }
+}


I'm not sure if much is gained from moving this into a separate json file, especially since it gets destructured in the test file. Probably not worth another refactor though.

It feels a bit like maybe some of the tests should just take JSON objects directly as arguments to the sortRoutes func. Everything feels a bit scattered by having to check which routes are which back in this file. It seems like a few of the tests have become incorrectly named as a result of this disjointedness.

landonreed · 2020-02-04T21:27:16Z

__tests__/util/__snapshots__/itinerary.js.snap

+    "longName": "Loop route",
+    "mode": "BUS",
+    "shortName": "2",
+    "sortOrder": -999,


I think this test might need to be renamed?

landonreed · 2020-02-04T21:54:32Z

__tests__/util/__snapshots__/itinerary.js.snap

+    "longName": "Zig-zagging route",
+    "mode": "BUS",
+    "shortName": "30",
+    "sortOrder": 2,


Test name mismatch?

landonreed

Looks really good. I just noted a few concerns about test name not quite matching the input data. This might be helped by placing the JSON test data directly into the test args rather than importing from the JSON file and referencing by variable names, but I'll leave that up to you.

binh-dam-ibigroup

Just one change to the 30X express test route!

binh-dam-ibigroup · 2020-02-03T15:30:21Z

lib/util/itinerary.js

+    // b is a valid number, b gets priority
+    if (isNaN(aVal)) return 1
+    // a is a valid number, a gets priority
+    if (isNaN(bVal)) return -1


@landonreed and @evansiroky: Where would this case fit?
{ shortName: "10C" }

binh-dam-ibigroup · 2020-02-04T22:14:16Z

__tests__/util/__mocks__/itinerary.json

+  "route9": {
+    "longName": "Express route",
+    "mode": "BUS",
+    "shortName": "30",


Name this one with shortName "30X" to see how it will get sorted!

evansiroky · 2020-02-05T02:05:38Z

Just pushed a commit that renames a test and removes one that's no longer needed.

evansiroky · 2020-02-05T02:14:10Z

...and I added a test case to prove that alphanumeric short names seem to get sorted properly.

evansiroky · 2020-02-14T16:17:30Z

🎉 This PR is included in version 0.13.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

refactor(route-viewer): make route comparator tolerate all kinds of data

063439a

evansiroky assigned landonreed and binh-dam-ibigroup Jan 28, 2020

binh-dam-ibigroup previously approved these changes Jan 28, 2020

View reviewed changes

landonreed reviewed Jan 28, 2020

View reviewed changes

landonreed assigned evansiroky and unassigned landonreed and binh-dam-ibigroup Jan 28, 2020

refactor(itinerary): improve route comparator functions

0d49d08

landonreed reviewed Jan 29, 2020

View reviewed changes

__tests__/util/itinerary.js Outdated Show resolved Hide resolved

landonreed reviewed Jan 29, 2020

View reviewed changes

lib/util/itinerary.js Outdated Show resolved Hide resolved

landonreed reviewed Jan 29, 2020

View reviewed changes

lib/util/itinerary.js Outdated Show resolved Hide resolved

landonreed reviewed Jan 29, 2020

View reviewed changes

landonreed requested changes Jan 29, 2020

View reviewed changes

refactor: address PR comments

9186539

rename some functions, use consistent arguments and allow disabling integer route short name sorting

evansiroky assigned landonreed and unassigned evansiroky Jan 30, 2020

binh-dam-ibigroup self-requested a review January 30, 2020 15:48

landonreed assigned evansiroky and unassigned landonreed Jan 30, 2020

binh-dam-ibigroup reviewed Jan 30, 2020

View reviewed changes

__tests__/util/itinerary.js Outdated Show resolved Hide resolved

evansiroky added 3 commits January 31, 2020 19:14

Add route sort comparator for route type

8fa6088

refactor: change order of some route types

8e481e9

refactor: add alphabetic shortName comparator, always use integer sho…

4fdc1d2

…rtName criteria

evansiroky removed their assignment Feb 1, 2020

evansiroky assigned landonreed and binh-dam-ibigroup Feb 1, 2020

landonreed reviewed Feb 4, 2020

View reviewed changes

landonreed assigned evansiroky and unassigned landonreed and binh-dam-ibigroup Feb 4, 2020

landonreed approved these changes Feb 4, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup Feb 4, 2020

binh-dam-ibigroup requested changes Feb 4, 2020

View reviewed changes

refactor: rename a test and remove a test that's no longer needed

3a46a3d

evansiroky assigned landonreed and unassigned evansiroky Feb 5, 2020

test: add test case with alphanumeric shortNames

f9f208d

binh-dam-ibigroup approved these changes Feb 5, 2020

View reviewed changes

binh-dam-ibigroup merged commit 316cc8d into fix-route-order Feb 5, 2020

landonreed mentioned this pull request Feb 13, 2020

Next Release #126

Merged

evansiroky added the released label Feb 14, 2020

evansiroky added the ported-to-otp-ui Indicates whether changes made to the maintenance branch have been ported to otp-ui label Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make route comparator tolerate all kinds of data #124

Make route comparator tolerate all kinds of data #124

evansiroky commented Jan 28, 2020

landonreed Jan 28, 2020

evansiroky Jan 28, 2020

landonreed Jan 28, 2020

codecov-io commented Jan 29, 2020 •

edited

Loading

evansiroky commented Jan 29, 2020

landonreed Jan 29, 2020

evansiroky Jan 30, 2020

landonreed Jan 30, 2020 •

edited

Loading

binh-dam-ibigroup Jan 30, 2020

evansiroky Feb 1, 2020

binh-dam-ibigroup Feb 3, 2020

landonreed Jan 29, 2020

landonreed left a comment

evansiroky commented Jan 30, 2020

landonreed Feb 4, 2020

landonreed Feb 4, 2020

landonreed Feb 4, 2020

landonreed Feb 4, 2020

landonreed left a comment

binh-dam-ibigroup left a comment

binh-dam-ibigroup Feb 3, 2020

binh-dam-ibigroup Feb 4, 2020

evansiroky commented Feb 5, 2020

evansiroky commented Feb 5, 2020

evansiroky commented Feb 14, 2020

Make route comparator tolerate all kinds of data #124

Make route comparator tolerate all kinds of data #124

Conversation

evansiroky commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 29, 2020 • edited Loading

Codecov Report

evansiroky commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed left a comment

Choose a reason for hiding this comment

evansiroky commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansiroky commented Feb 5, 2020

evansiroky commented Feb 5, 2020

evansiroky commented Feb 14, 2020

codecov-io commented Jan 29, 2020 •

edited

Loading

landonreed Jan 30, 2020 •

edited

Loading