salvaging that no longer used endpoint.

@ardumont have a look here... see... that's why it fails ;)

same here, it's OriginVisitStatus you want ;)

why the name page_name?

assert date_visit1 < date_visit2 < date_visit3

i don't know, i kept the naming logic from the function just before.
i adapted the value from pagination to token as you seemed keen on that (from the test).

What do you propose instead (nothing comes to mind for my part)?

It's related to pagination so page for short.

do we really need those? I mean, these are type annotated, so it seems unnecessary to me to add execution time type validation.

a small comment on the reason for this +1 could be handy (aka get one more result to handle the next_page logic).

Also not sure this extra_limit variable is really helpful, as well as the assertion below seems unnecessary to me (add unit tests for _cql_runner.origin_visit_status_get_range is needed, then trust them).

this is wrong, the next_page_token should be before the visit_statuses above.

long story short, here is my proposal for this part of the code:

def origin_visit_status_get([...]):
    next_page_token = None
    date_from = None
    if page_token is not None:
        date_from = datetime.datetime.fromisoformat(page_token)
    rows = self._cql_runner.origin_visit_status_get_range(
        origin, visit, date_from, limit+1, order
    )
    visit_statuses = [converters.row_to_visit_status(row) for row in rows[:limit]]
    if len(rows) > limit:
        next_page_token = str(converters.row_to_visit_status(rows[limit+1].date)
    return PagedResult(results=visit_statuses, next_page_token=next_page_token)

except until recently it was moot...
D3635 fixed it...

But that reassures me a bit to have those ;)

i use the extra-limit variable to avoid having + 1 all over the places which i found annoying to read and prone to error.

tests disagrees there.

let's see if that works ;)

Note: this applies to other storage implementations too, i guess.

then either tests are wrong or the limit+1 is useless, since you never use the extra returned value...

I'm using it but not for its value for its presence.

but it adds code and (at least) "visual complexity", so I'd rather say "no", but I won't fight too hard, it's not a big deal, really.

but like i said, i'll try your version which seems simpler to read and to the point alright ;)

You can avoid multiple +1 without extra_limit:

        rows = self._cql_runner.origin_visit_status_get_range(
            origin, visit, date_from, limit+1, order
        )
...
        if len(visit_statuses) > limit:
...

It's not just the test that disagrees. next_page_token is the date of the last returned visit, and the next call will return all visits with a date strictly greater than the one in the token

if you do that, then you need to change the inequalities in the backend to be non-strict inequalities

ok then this MUST be documented somehow because it really feels wrong for the fellow dev reading this piece of code.

BTW, do we really need to gather this extra result from the base? why not just add the next link if len(rows) == limit (according we do not use limit+1 when calling it).

Worst case, we do an extra call with no results if the total number of visits is a multiple of limit.

oh and your code is incorrect david, because you're returning the limit first results, and use the limit+2's date as token

yes it's indeed incorrect since I did not get the behavior of _cql_runner.origin_visit_status_get_range() w.r.t the date_from argument. ( I mean calling a function with a date_from argument, I expect to have results starting from this date by default).
So I say this should be explained in a oneliner comment.

But as said above, I'm not sure I see the real advantage of gathering limit+1 results from the base (just add the next_page_token iff len(rows) == limit )

It does not work for the reasons val exposed.

either the tests are wrong...

I think the tests are fine, spent quite some time on it (either here and for origin-visit-get endpoint).

In the end, the improvments i can do here is:

• using the list comprehension proposed.
• drop the extra check on the argument (plus it's not consistent, we don't do it on all parameters)

Note:
len(rows) does not work, rows is a ResultSet and they complain about it (mypy and tests).

( I mean calling a function with a date_from argument, I expect to have results starting from this date by default).

ah yes, that's reasonable.

(just add the next_page_token iff len(rows) == limit

That saves one call to the api from the user perspective?
Here we know and say as much, there is still something to iterate again over.

without the extra limit, i gather, the user needs to make one more call with possibly nothing to retrieve.

ah yes, that's reasonable.

I'll check how hard it is to invert the strictness in the backend...
A priori, i should not have anything to change in the tests...
and in the end, everything should stay green... i think

That saves one call to the api from the user perspective?

and David's argument is that it's not worth the extra complexity + fetching an extra record.

Personally I don't care either way.

Yes, but I'm more for the "extra complexity" argument, as you would have guessed ;-) But I don't really care either as long as the code is easy to read and understand (i.e. with comments for traps like the date_from argument)

I'll try something...

date_name = "date"
...
date_name = "no_date"

(i.e. with comments for traps like the date_from argument)

I successfully inverted the strictness.
So it's no longer a trap, we include the date as starting point.

(And note that i did not realize it was a trap, otherwise, i would have done something about it...)

Though that means we need to exclude the last result from the fetch.
So that means we need the +1 to fetch that result but not returning it. (to respect the limit number,)

We use its date as value for the next-page-token (so it is used!).

i'll check the extra comments you made to adapt according before pushing an update.

Note: I can remove the extra-limit though and just use limit + 1 etc...