Page MenuHomeSoftware Heritage

Add ?limit=N method variants to return first N results

Authored by legau on Feb 13 2020, 12:43 PM.

Diff Detail

rDGRPH Graph service
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

legau created this revision.Feb 13 2020, 12:43 PM
vlorentz requested changes to this revision.Feb 14 2020, 10:39 AM
vlorentz added subscribers: seirl, zack, vlorentz.

@zack @seirl Could either of you have a look too?


'-1' for consistent typing


Can't count be strictly lower than limit?

If yes, you should add checks for this case in tests.

This revision now requires changes to proceed.Feb 14 2020, 10:39 AM
legau marked an inline comment as done.Feb 14 2020, 2:23 PM
legau added inline comments.

It can if last is True. But there's still the open question of what to do with the /last endpoint as ?limit is supposed to be a generalization. So until then I don't know what this should be.

zack added inline comments.Feb 14 2020, 2:59 PM

You keep saying/asking that, but the easy hack you're referring to already clarifies in its description that last is not a generalization of limit. I suggest you reread the task description.

legau added inline comments.Feb 14 2020, 3:05 PM

I'm saying the contrary, that limit is a generalization of last, which is I think what is written in the task description. Does that mean a /last?limit=N returns the last N nodes ?

seirl added inline comments.Feb 14 2020, 5:19 PM

The task description specifies that ?limit is for the head, not the tail.

If you want to generalize that to the tail and replace /last, maybe you could use positive numbers for head limits and negative numbers for tail limits?

  • ?limit=0 -> No limit
  • ?limit=10 -> First 10 results
  • ?limit=-10 -> Last 10 results?

This is consistent with what Python uses for indexing negative numbers, but I don't know if it's a good API.

legau added inline comments.Feb 17 2020, 1:52 PM

I had the same idea and it's true that it seems weird for an API but why not.
I see two other possible implementations :
Use ?limit only on non /last endpoint.
Use ?limit with /last the same way as non /last but on tail not head.

Tell me what way you'd prefer.

zack added inline comments.Feb 17 2020, 1:58 PM

I'm fine with the positive/negative limit idea.
(And once we have it, we can drop /last )

legau updated this revision to Diff 9570.Feb 17 2020, 3:19 PM
  • limit param now goes head and tail
legau marked 4 inline comments as done.Feb 17 2020, 3:21 PM
vlorentz accepted this revision.Feb 21 2020, 12:23 PM

Looks good to me, thanks!

This revision is now accepted and ready to land.Feb 21 2020, 12:23 PM
legau updated this revision to Diff 9681.Feb 24 2020, 6:19 PM

Squash commits

legau added a comment.Mar 2 2020, 10:58 AM

I'm not able to push to master.