Skip to content
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

DEVEXP-411: Add support for Access control List #77

Conversation

asein-sinch
Copy link
Collaborator

No description provided.

@asein-sinch asein-sinch requested a review from a team April 29, 2024 08:06
Copy link

@JPPortier JPPortier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

const requestOptionsPromise = this.client.prepareOptions(basePathUrl, 'GET', getParams, headers, body || undefined);

const operationProperties: PaginatedApiProperties = {
pagination: PaginationEnum.PAGE2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR (see comment from another PR related to pagination handling from client/core)
PaginationEnum.PAGE2, PAGE3, ... are not very intuitive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree: at the beginning, there was only TOKEN and PAGE, but then I noticed that the parameters for the PAGE pagination are different from one API to the other and even the start page is not the same. So as Fax is v3, I thought the APIs were following a kind of evolution and I created a new pagination type called PAGE3.
And then I saw this one and I was out of ideas to find a name that would characterise the pagination behaviour. So I went for the opportunistic PAGE2 that was conveniently filling the gap 😅

@asein-sinch asein-sinch merged commit ac3183d into DEVEXP-406_Add-support-for-SIP-Trunks May 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants