feat(csharp): add pagination helper methods to C# SDK generator #5187

Swimburger · 2024-11-15T23:00:36Z

This feature works, but the code is still a little rough and needs some cleaning.

The new HttpPagerEndpointGenerator generates pager methods that use the main endpoint methods for users to paginate over items in a paginated endpoint.
AsIs/Page.Template.cs holds a list of items in a single page
AsIs/Pager.Template.cs has a public abstract Pager<Item> class for a user to interact with, and two internal implementations OffsetPager and CursorPager`. The bulk of the logic to paginate is stored within these implementations, and the remaining parts are passed into the pagers as delegates.

⚠️ Assumption: For cursor-based pagination, the cursor is assumed to be a string. This is probably correct in 95% of cases, but that's just a guess. Does the cursor type need to be configurable?

⚠️ Big change: The generator thus far assumes that each endpoint generates one method on a client. This PR changes that so that an endpoint generator can generate multiple methods. For generated SDKs, this isn't too big of a change, but this also affects generated snippets and references. There's some code duplication to quickly get snippets and references to work as expected with this pager variation method for the endpoint

Should we stick to one method per endpoint to reduce the amount of change in this generator and downstream dependencies? Or
Should we push forward on an endpoint with multiple methods and refactor the generator at a larger scale for this to be supported seamlessly?

…harp/pagination

* Refactor to take into account different RequestOptions * Generate snippets * Generate reference

github-actions · 2024-11-15T23:02:53Z

🌿 Preview your docs: https://fern-preview-c360b9f1-650a-46ff-bc3a-99fc70f3cefc.docs.buildwithfern.com/learn

Swimburger · 2024-11-18T16:11:11Z

Decisions:

Make original method private/internal
Only make pager public and use the original method name
Produce snippets/references only with pager name

Swimburger · 2024-11-18T19:31:16Z

The mockResponse is generated incorrectly in ListWithExtendedResultsAndOptionalDataTest and ListWithExtendedResultsTest.
The mockresponse JSON should have the data property. The list within data is optional so that's fine, but there should be a data: {} property since that isn't optional in Fern definition.

* Support different types in cursor pagination * Support int and long in offset pagination * Initialize request if null and nested properties towards setCursor and setOffset

Swimburger · 2024-11-18T23:58:27Z

Improvements:

Support for different types in cursor pagination
Support for int and long in offset pagination
Initialize request if null before passing request to setCursor and setOffset
Initialize nested properties towards cursor and offset property.

For initializing towards cursor and offset property, I assume the types don't have required fields and initialize an empty instance using ?? = new().

I'm worried if one of these types does have a required field, at which point the compiler will complain.

amckinney

Overall looking great - mostly comments on implementation, and only minor details for the generated code (e.g. comments, etc).

amckinney · 2024-11-19T00:16:39Z

generators/csharp/sdk/src/endpoint/AbstractEndpointGenerator.ts

+                offset: (pagination) => pagination.results.property.valueType,
+                cursor: (pagination) => pagination.results.property.valueType,
+                _other: (pagination) => {
+                    throw new Error(`Unsupported pagination type: ${pagination.type}`);


Prefer switch w/ assertNever if possible.

generators/csharp/sdk/src/endpoint/EndpointGenerator.ts

amckinney · 2024-11-19T00:18:16Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+                    _other: (pagination) => {
+                        throw new Error(`Unsupported pagination type: ${pagination.type}`);
+                    }


nit: Prefer switch w/ assertNever here.

amckinney · 2024-11-19T00:18:44Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+                        writer.writeTextStatement(")");
+                        writer.writeTextStatement("return pager");
+                    },
+                    cursor: (pagination) => {


nit: Could we put all of this in a separate function (e.g. generateCursorPagerMethod).

amckinney · 2024-11-19T00:18:52Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+                }
+
+                endpoint.pagination?._visit({
+                    offset: (pagination) => {


nit: Could we put all of this in a separate function (e.g. generateOffsetPagerMethod).

amckinney · 2024-11-19T00:34:38Z

seed/csharp-sdk/pagination/src/SeedPagination/Users/UsersClient.cs

+        var pager = new CursorPager<
+            ListUsersBodyCursorPaginationRequest,
+            RequestOptions?,
+            ListUsersPaginationResponse,
+            string,
+            User
+        >(
+            request,
+            options,
+            ListWithBodyCursorPaginationAsync,
+            (request, cursor) =>
+            {
+                request.Pagination ??= new();
+                request.Pagination.Cursor = cursor;
+            },
+            response => response?.Page?.Next?.StartingAfter,
+            response => response?.Data?.ToList()
+        );
+        return pager;
+    }


The generated code looks great 👏

generators/csharp/sdk/src/test-generation/mock-server/MockServerTestGenerator.ts

amckinney · 2024-11-19T00:37:25Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+    public generateEndpointSnippet({
+        example,
+        endpoint,
+        clientVariableName,
+        serviceId,
+        requestOptions,
+        getResult,
+        parseDatetimes
+    }: {
+        example: ExampleEndpointCall;
+        endpoint: HttpEndpoint;
+        clientVariableName: string;
+        serviceId: ServiceId;
+        requestOptions?: csharp.CodeBlock;
+        getResult?: boolean;
+        parseDatetimes: boolean;
+    }): csharp.MethodInvocation | undefined {
+        const additionalEndParameters = requestOptions != null ? [requestOptions] : [];
+        return this.hasPagination(endpoint)
+            ? this.generateHttpPagerEndpointSnippet({
+                  example,
+                  endpoint,
+                  clientVariableName,
+                  serviceId,
+                  additionalEndParameters,
+                  getResult,
+                  parseDatetimes
+              })
+            : this.generateHttpEndpointSnippet({
+                  example,
+                  endpoint,
+                  clientVariableName,
+                  serviceId,
+                  additionalEndParameters,
+                  getResult,
+                  parseDatetimes
+              });
+    }


amckinney · 2024-11-19T00:40:02Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+        const endpointSignatureInfo = this.getEndpointSignatureInfo({ serviceId, endpoint });
+        const parameters = [...endpointSignatureInfo.baseParameters];
+        const optionsParamName = this.getRequestOptionsParamNameForEndpoint({ endpoint });
+        const requestOptionsParam = this.getRequestOptionsParameter({ endpoint });
+        const requestOptionsType = requestOptionsParam.type;
+        parameters.push(requestOptionsParam);
+        const itemType = this.getPaginationItemType(endpoint);
+        const return_ = this.getPagerReturnType(endpoint);
+        const snippet = this.getHttpPagerMethodSnippet({ endpoint });


There's a lot of parameters required here - I wonder if it's be worth rolling out a PaginatedEndpointGenerator class (or something similar) that could define all these properties as member variables, so that the following implementation could be reduced / a little more concise and readable.

Not totally necessary to do that here, but let me know your thoughts.

generators/csharp/sdk/src/endpoint/AbstractEndpointGenerator.ts

Swimburger · 2024-11-19T02:59:32Z

I added a shallow clone using the record with {} feature. Future versions of C# may include a DeepClone feature for records.

amckinney

Nice!! 🎉

.vscode/settings.json

generators/csharp/codegen/src/asIs/Pager.Template.cs

generators/csharp/sdk/src/endpoint/AbstractEndpointGenerator.ts

amckinney · 2024-11-19T14:25:59Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+        writer.write("var pager = ");
+        writer.writeNodeStatement(
+            csharp.instantiateClass({
+                classReference: cursorPagerClassReference,
+                arguments_: [
+                    csharp.codeblock(requestParam.name),
+                    csharp.codeblock(optionsParamName),
+                    csharp.codeblock(unpagedEndpointMethodName),
+                    csharp.codeblock((writer) => {
+                        writer.writeLine("(request, cursor) => {");
+                        writer.indent();
+                        this.initializeNestedObjects(writer, "request", pagination.page);
+                        writer.writeTextStatement(`${this.dotGet("request", pagination.page)} = cursor`);
+                        writer.dedent();
+                        writer.writeLine("}");
+                    }),
+                    csharp.codeblock(`response => ${this.nullableDotGet("response", pagination.next)}`),
+                    csharp.codeblock(`response => ${this.nullableDotGet("response", pagination.results)}?.ToList()`)
+                ]
+            })
+        );
+        writer.writeTextStatement("return pager");


nit: Should we just return the pager directly rather than using var?

amckinney · 2024-11-19T14:26:25Z

generators/csharp/sdk/src/endpoint/http/HttpEndpointGenerator.ts

+        writer.write("var pager = ");
+        writer.writeNodeStatement(
+            csharp.instantiateClass({
+                classReference: offsetPagerClassReference,
+                arguments_: [
+                    csharp.codeblock(requestParam.name),
+                    csharp.codeblock(optionsParamName),
+                    csharp.codeblock(unpagedEndpointMethodName),
+                    csharp.codeblock(`request => ${this.nullableDotGet("request", pagination.page)} ?? 0`),
+                    csharp.codeblock((writer) => {
+                        writer.writeLine("(request, offset) => {");
+                        writer.indent();
+                        this.initializeNestedObjects(writer, "request", pagination.page);
+                        writer.writeTextStatement(`${this.dotGet("request", pagination.page)} = offset`);
+                        writer.dedent();
+                        writer.writeLine("}");
+                    }),
+                    csharp.codeblock(
+                        pagination.step ? `request => ${this.nullableDotGet("request", pagination.step)} ?? 0` : "null"
+                    ),
+                    csharp.codeblock(`response => ${this.nullableDotGet("response", pagination.results)}?.ToList()`),
+                    csharp.codeblock(
+                        pagination.hasNextPage
+                            ? `response => ${this.nullableDotGet("response", pagination.hasNextPage)}`
+                            : "null"
+                    )
+                ]
+            })
+        );
+        writer.writeTextStatement("return pager");


nit: Same thing with returning directly.

Co-authored-by: Alex McKinney <alexmckinney01@gmail.com>

…/fern into niels/csharp/pagination

Swimburger added 3 commits November 15, 2024 01:57

WIP: Generate pager endpoints

f3c4651

Merge branch 'main' of https://github.com/fern-api/fern into niels/cs…

80b675a

…harp/pagination

feat(csharp): Refactor Pager (WIP)

10ce7c3

* Refactor to take into account different RequestOptions * Generate snippets * Generate reference

Swimburger added 4 commits November 18, 2024 11:30

Fix HttpPagerEndpointGenerator namespace name

e4721bd

Refactor C# pager generation to have a single public method

1699ca3

Remove pagination if no endpoints have pagination

832c1f1

C# mockserver test pagination

28ae905

Swimburger marked this pull request as ready for review November 18, 2024 19:32

Swimburger requested a review from dsinghvi as a code owner November 18, 2024 19:32

Swimburger added 3 commits November 18, 2024 14:33

Merge branch 'main' into niels/csharp/pagination

3dedd98

format

3f976a6

Fix pnpm format

bfd7f43

Swimburger changed the title ~~feat(csharp): WIP add pagination helper methods to C# SDK generator~~ feat(csharp): add pagination helper methods to C# SDK generator Nov 18, 2024

Improve pagination

88b265f

* Support different types in cursor pagination * Support int and long in offset pagination * Initialize request if null and nested properties towards setCursor and setOffset

amckinney reviewed Nov 19, 2024

View reviewed changes

Swimburger added 3 commits November 18, 2024 20:56

Incorporate feedback + revert pagination seed test definition

f09d4f2

Undo cached seed definition change

59dbb9b

clone request before paginating

baee9f0

Swimburger requested a review from amckinney November 19, 2024 02:59

Swimburger and others added 3 commits November 18, 2024 22:02

fix eslint issue

75e1c63

Add version to C# SDK versions.yml

4adbe53

chore: update changelog

550cfd4

amckinney approved these changes Nov 19, 2024

View reviewed changes

Swimburger and others added 2 commits November 19, 2024 11:01

Apply suggestions from code review

017c1ea

Co-authored-by: Alex McKinney <alexmckinney01@gmail.com>

Undo .vscode settings.json change

ec17568

Swimburger added 3 commits November 19, 2024 11:03

Merge branch 'niels/csharp/pagination' of https://github.com/fern-api…

3b8e766

…/fern into niels/csharp/pagination

Add test cases for pagination

b680a29

Fix writing test files to correct location

e5d5bde

Swimburger enabled auto-merge (squash) November 19, 2024 20:59

Swimburger merged commit 339cc01 into main Nov 19, 2024
57 of 59 checks passed

Swimburger deleted the niels/csharp/pagination branch November 19, 2024 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(csharp): add pagination helper methods to C# SDK generator #5187

feat(csharp): add pagination helper methods to C# SDK generator #5187

Swimburger commented Nov 15, 2024

github-actions bot commented Nov 15, 2024 •

edited

Loading

Swimburger commented Nov 18, 2024

Swimburger commented Nov 18, 2024

Swimburger commented Nov 18, 2024

amckinney left a comment

amckinney Nov 19, 2024

amckinney Nov 19, 2024

amckinney Nov 19, 2024

amckinney Nov 19, 2024

amckinney Nov 19, 2024

amckinney Nov 19, 2024

amckinney Nov 19, 2024

Swimburger commented Nov 19, 2024

amckinney left a comment

amckinney Nov 19, 2024

amckinney Nov 19, 2024

feat(csharp): add pagination helper methods to C# SDK generator #5187

feat(csharp): add pagination helper methods to C# SDK generator #5187

Conversation

Swimburger commented Nov 15, 2024

github-actions bot commented Nov 15, 2024 • edited Loading

Swimburger commented Nov 18, 2024

Swimburger commented Nov 18, 2024

Swimburger commented Nov 18, 2024

amckinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swimburger commented Nov 19, 2024

amckinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2024 •

edited

Loading