Page MenuHomeSoftware Heritage

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

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

Details

Summary

Implementation of new query param ?limit=N on walks (T2114)

Diff Detail

Repository
rDGRPH Graph service
Branch
limit-param
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10631
Build 15919: tox-on-jenkinsJenkins
Build 15918: arc lint + arc unit

Event Timeline

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

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

swh/graph/server/app.py
90

'-1' for consistent typing

164

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.Fri, Feb 14, 10:39 AM
legau marked an inline comment as done.Fri, Feb 14, 2:23 PM
legau added inline comments.
swh/graph/server/app.py
164

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.Fri, Feb 14, 2:59 PM
swh/graph/server/app.py
164

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.Fri, Feb 14, 3:05 PM
swh/graph/server/app.py
164

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.Fri, Feb 14, 5:19 PM
swh/graph/server/app.py
164

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.Mon, Feb 17, 1:52 PM
swh/graph/server/app.py
164

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.Mon, Feb 17, 1:58 PM
swh/graph/server/app.py
164

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.Mon, Feb 17, 3:19 PM
  • limit param now goes head and tail
legau marked 4 inline comments as done.Mon, Feb 17, 3:21 PM
vlorentz accepted this revision.Fri, Feb 21, 12:23 PM

Looks good to me, thanks!

This revision is now accepted and ready to land.Fri, Feb 21, 12:23 PM