search

OpendataCH / Transport

Swiss public transport API
http://transport.opendata.ch/
MIT License
241 stars 50 forks source link

Prevent overlaps in pagination between future and past pages (#158) #185

Closed CedricReichenbach closed 6 years ago

CedricReichenbach commented 6 years ago

The currently utilized fahrplan.search.ch API treats the "num" and "pre" parameters in a way such that they overlap by 4 connections:

    | num --->
   <--- pre |
-o-o-o-o-o-o-o-o-o-o-
            ^ time

In order to respect this, we now skip the first 4 previous items, i.e. our page -1 starts at "pre=4".

@helbling Can you verify this is expected behavior and correct consumption of the API?

An integration test verifying this (and preventing regression) would be great. But from what I can tell, no such tests (connecting to the external API) are in place yet, and I'm not familiar enough with the stack to introduce them.

CedricReichenbach commented 6 years ago

I saw there were some style issues (independent of this PR) and took the liberty to add a commit on top fixing those.

Now that everything's green, what's the plan on merging this PR?

It's probably safe to assume that num/pre behavior on search.ch side won't change anytime soon, given it hasn't done so since summer. So this should be fairly safe from regression.

fabian commented 6 years ago

@CedricReichenbach Are you sure the original problem you're describing still exists? Can you provide two example queries where you see it?

I can't see any overlap with the following two queries:

CedricReichenbach commented 6 years ago

@fabian Yes, it still exists.

However, it only affects arrival time search, which I somehow forgot to consider in my patch. Now it should be correct, i.e. only carry out the special treatment for arrival searches.

The situation is outlined in a bit more detail here: https://github.com/OpendataCH/Transport/issues/158#issuecomment-346999686

An example pair of overlaps is:

Those two serve the exact same list of connections.

fabian commented 6 years ago

Okay, I think this makes sense, as explained in https://github.com/OpendataCH/Transport/issues/158#issuecomment-346810080:

Note that the default for time_type=arrival is pre=4&num=0