-
Notifications
You must be signed in to change notification settings - Fork 629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
com.atproto.sync.listReposByCollection Lexicon, for collections directory #3524
Conversation
"type": "integer", | ||
"minimum": 1, | ||
"maximum": 500, | ||
"default": 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the default should be much higher, paging through millions at 50/call would be bad. The db access is super cheap, this is just about what a reasonable network blob is. 32 byte did * 1000 would only be 32kB of data so at least that. Maybe default 500 or 1000, upper limit 5000 or 10000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a common case is going to be folks just iterating through hundreds of accounts, not millions, right? eg, looking for all feed generator declarations, or statusphere users. I think folks doing this bulk should configure the limit; it is nice to have it smaller for devs poking around with curl
.
I'll bump to default=100 and max=5000, and add a description recommending larger.
(I don't feel super strongly about this and could bump higher as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My inclination would be to follow sync.listRepos
which has a pretty similar use case. The number of repos on a host vs. within a collection seems like they both span large and small lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync.listRepos
has min=1, default=500, max=1000.
I went with min=1, default=500, max=2000 to be sorta-aligned
@bnewbold if you do a codegen, please also add a changeset for |
53cc03a
to
8afec3f
Compare
Pairs with: bluesky-social/indigo#929
Have not run codegen in this repo; usually do that just before merge (after approval).
There was some back and forth on whether we should return array-of-objects or just array-of-strings (DIDs). I think it is good to have this more consistent with
listRepos
, which returns array-of-objects.@brianolson had a minimum bound on the
limit
parameter (50). For consistency with other endpoints, I made the Lexicon minimum here 1.This is just the
listReposByCollection
endpoint; I didn't do a Lexicon forcom.atproto.sync.countReposByCollection
.