Add an API endpoint for retrieving a route's shape

[arkadyan]

Save shape data from GTFS.

Preliminary step for Shuttle Map | Show lines (no stops) for selected shuttle routes (shapes on the map)

[codecov]

Codecov Report

Merging #201 into master will decrease coverage by <.01%.
The diff coverage is 95.45%.

@@            Coverage Diff            @@
##           master    #201      +/-   ##
=========================================
- Coverage    95.8%   95.8%   -0.01%     
=========================================
  Files         115     117       +2     
  Lines        2504    2526      +22     
  Branches      275     275              
=========================================
+ Hits         2399    2420      +21     
- Misses        103     104       +1     
  Partials        2       2

Impacted Files	Coverage Δ
lib/gtfs/trip.ex	100% <ø> (ø)	⬆️
lib/skate_web/controllers/shape_controller.ex	100% <100%> (ø)
lib/skate_web/router.ex	100% <100%> (ø)	⬆️
lib/gtfs/data.ex	100% <100%> (ø)	⬆️
lib/gtfs/shape.ex	100% <100%> (ø)
lib/gtfs.ex	61.29% <75%> (+0.94%)	⬆️

---

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 067cd4f...7e39177. Read the comment docs.

[skyqrose]

Having sequence is needed in the csv because you don't have order guarantees, but in elixir, I think we can just put these in an ordered list and not store this field.

If we don't store this field, we might be able to have a much simpler format in shape.ex which could let us get rid of shape_point.ex entirely:

# shape.ex
  @type t :: %__MODULE__{
          id: id(),
          points: [{float(), float()}]
        }

[arkadyan]

How do we know that the points coming in are in proper order?

Even if sequence goes away I wouldn't want to lose the naming of lat and lon, I think that is very valuable.

[skyqrose]

You could sort by sequence during the loading process, but then not store it. Similar to how we check route_type in routes when loading but then don't store it.

Ah, yeah I see your point about the names. Could the struct/map with lat/lon naming live in shape.ex? It's so related to Shape that I don't think it really stands well enough on its own to have its own separate file.

[arkadyan]

I think that is stymied by the fact that we have to pull in one point at a time, and only later are able to assemble them into a shape.

[skyqrose]

The existing parse helper in csv.ex uses a simple map, so wouldn't be able to use it, but we can do the sort if we're okay some more custom parsing code.

...
|> CSV.decode!(headers: true) # [map()]
|> Enum.group_by(fn row -> row["shape_id"] end) # %{shape_id() => map()}
|> Map.new(fn {shape_id, rows_in_shape} ->
  points =
    rows_in_shape
      |> Enum.sort_by(fn row -> Integer.parse(row["sequence"]) end)
      |> Enum.map(fn row -> %{lat: ..., lon: ...} end)
  {shape_id, %Shape{
    id: shape_id,
    points: points
  }}
end)

It is more complicated parsing than the simple one struct per row, but I think that having a cleaner data type is more important than simple parsing.

[arkadyan]

I disagree. I think the existence of that property is causing no harm and having custom parsing code would make things more complicated and less extensible. Plus I think it would be important to have a type for that intermediate step anyway so you'd still have the same description of the sequence property.

[skyqrose]

Having an unnecessary complicated data model does do harm, in that it adds complexity to every place that uses shapes. We'd have to add the sequence field whenever we make test data. We have to use more bandwidth and memory to store it. There are new meaningless/impossible states, like %{points: %{sequence: 2}, %{sequence: 1}}. It'd be better to clear out that complexity once in the parsing and have a data model that represents what we want, rather than keep it around for every other place that uses shapes.

[arkadyan]

The point sorting function is actually wrong. Will fix momentarily.

[skyqrose]

A couple small non-blocking comments.

[skyqrose]

This is a lot of shape-specific code to have in data.ex. Is there any of it that would make sense to move to shape.ex so that data.ex can stay a little smaller?

[skyqrose]

Would it make sense to have ShapePoint as a submodule inside this file, so it's more closely associated with the Shape module? I don't think we'd ever want to use a ShapePoint outside of the context of a Shape.