Page MenuHomeSoftware Heritage

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

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

Diff Detail

Repository
rDGRPH Compressed graph representation
Branch
limit-param
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10746
Build 16133: tox-on-jenkinsJenkins
Build 16132: arc lint + arc unit

Event Timeline

vlorentz added subscribers: seirl, zack, vlorentz.

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

swh/graph/server/app.py
89

'-1' for consistent typing

163

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 added inline comments.
swh/graph/server/app.py
163

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.

swh/graph/server/app.py
163

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.

swh/graph/server/app.py
163

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 ?

swh/graph/server/app.py
163

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.

swh/graph/server/app.py
163

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.

swh/graph/server/app.py
163

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

  • limit param now goes head and tail

Looks good to me, thanks!

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

I'm not able to push to master.