Conversation
cmd/lk/phone_number.go
Outdated
| return nil | ||
| } | ||
|
|
||
| return listAndPrint(ctx, cmd, func(ctx context.Context, req *livekit.SearchPhoneNumbersRequest) (*livekit.SearchPhoneNumbersResponse, error) { |
There was a problem hiding this comment.
nit: this is a bit hard to read. any chance to break it out?
cmd/lk/phone_number.go
Outdated
| var ( | ||
| PhoneNumberCommands = []*cli.Command{ | ||
| { | ||
| Name: "phonenumber", |
There was a problem hiding this comment.
should we just use number for short? wdyt?
There was a problem hiding this comment.
yeah, that works well for the cli. Want to call out that the apis use phonenumber though.
…endering block more readable
rektdeckard
left a comment
rektdeckard
left a comment
There was a problem hiding this comment.
LGTM! Minor comments on naming
cmd/lk/phone_number.go
Outdated
| &cli.IntFlag{ | ||
| Name: "limit", | ||
| Usage: "Maximum number of results (default: 50)", | ||
| Value: 50, | ||
| }, | ||
| &cli.StringFlag{ | ||
| Name: "page-token", | ||
| Usage: "Token for pagination (empty for first page)", | ||
| }, |
There was a problem hiding this comment.
I think this is the best solution for now, but we were discussing either running implicit (behind the scenes) pagination (example here: #694) or doing a TUI pager that would abstract away having to find and pass an explicit page-token.
As it stands, will I need to format my results as --json to see the page token in order to be able to fetch the next page?
There was a problem hiding this comment.
I mention it here because we can be pretty sure that at least a few of our customers will have hundreds of numbers.
There was a problem hiding this comment.
So I haven't implemented pagination fully in the backend. I'm going to remove this param for now, fix it in the backend and then create another PR for pagination.
on the approach for pagination, for the implicit pagination, would we run into memory issues if we load large lists ? I guess I'd like to understand concerns against the page-token approach.
cmd/lk/phone_number.go
Outdated
| Usage: "Use phone number ID for direct lookup", | ||
| }, | ||
| &cli.StringFlag{ | ||
| Name: "phonenumber", |
cmd/lk/phone_number.go
Outdated
| Action: purchasePhoneNumbers, | ||
| Flags: []cli.Flag{ | ||
| &cli.StringSliceFlag{ | ||
| Name: "phonenumbers", |
There was a problem hiding this comment.
Nit: numbers is nicer, and how we refer to them in other SIP contexts
cmd/lk/phone_number.go
Outdated
| Usage: "Use phone number IDs for direct lookup", | ||
| }, | ||
| &cli.StringSliceFlag{ | ||
| Name: "phonenumbers", |
…nenumber(s) to number(s)
nishadmusthafa
force-pushed
the
phone-number-api
branch
from
de97c0c to
d21eca3
Compare
3 participants
No description provided.