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.

Sign up for GitHub

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

feat: Add datastore count API #93

Merged
merged 3 commits into from
Mar 13, 2024
Merged

feat: Add datastore count API #93

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3162134
feat: Add datastore count API
cyungslack Feb 27, 2024
3b1d1ba
Docs: Add datastore.count/query field descriptions
cyungslack Mar 11, 2024
3ea0b53
refactor: Put shared count and query args into DynamoQueryArgs type
cyungslack Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/typed-method-types/apps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,35 @@ export type DatastoreQueryResponse<
items: DatastoreItem<Schema>[];
};

export type DatastoreCountArgs<
Schema extends DatastoreSchema,
> =
& BaseMethodArgs
& {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the arguments to datastore.query and datastore.count share a lot of fields, yes? Like datastore, expression, attributes and values.

I recommend factoring out these shared fields into its own dedicated type (doesn't have to be exported, can just exist in this file), maybe something like a type DynamoQueryArgs, and have each of DatastoreQueryArgs and DatastoreCountArgs leverage that shared type.

This way, you don't duplicate all the work, and especially with JSDocs now being a part, don't have to duplicate the docs either!

Copy link
Contributor

Choose a reason for hiding this comment

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

For an example of how to do this, see the Node Slack SDK here: https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/types/request/conversations.ts#L52-L53

There are a bunch of non-exported interfaces in the above file, and they're mixed and combined together using | and & in typescript for different *Arguments types (like ConversationsAcceptSharedInviteArguments that I linked to). It is essentially the solution to the same problem we are solving in this PR.

/**
* @description The name of the datastore
*/
datastore: Schema["name"];
expression?: string;
Copy link
Contributor

@WilliamBergamin WilliamBergamin Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will the descriptions of these fields be included in the documentation?
Should we add descriptions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are descriptions in the documentation.
Here's the dev API page https://api.dev.slack.com/methods/apps.datastore.count which has short descriptions of the optional fields.
There's a much more in-depth explanation of the expression fields here (which should be re-used for the count explanation as the expression fields work the same way as query): https://api.slack.com/automation/datastores-retrieve#query

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do as much as we can to provide at least a basic description to specific fields. If there are relevant links for devs to read up on for more info, let's link them to it.

See for example how we do document fields and parameters and link to more information in our Node SDK: https://github.com/slackapi/node-slack-sdk/blob/main/packages/web-api/src/methods.ts#L1229-L1232

"expression_attributes"?: Record<string, string>;
"expression_values"?: Record<string, string | boolean | number>;
};

export type DatastoreCountResponse<
Schema extends DatastoreSchema,
> =
& BaseResponse
& {
/**
* @description The name of the datastore
*/
datastore: Schema["name"];
/**
* @description The number of items matching your query
*/
count: number;
};

export type DatastoreDeleteArgs<
Schema extends DatastoreSchema,
> =
Expand Down Expand Up @@ -293,6 +322,11 @@ export type AppsDatastoreQuery = {
args: DatastoreQueryArgs<Schema>,
): Promise<DatastoreQueryResponse<Schema>>;
};
export type AppsDatastoreCount = {
<Schema extends DatastoreSchema>(
args: DatastoreCountArgs<Schema>,
): Promise<DatastoreCountResponse<Schema>>;
};
export type AppsDatastoreDelete = {
<Schema extends DatastoreSchema>(
args: DatastoreDeleteArgs<Schema>,
Expand Down Expand Up @@ -368,6 +402,7 @@ export type TypedAppsMethodTypes = {
bulkPut: AppsDatastoreBulkPut;
update: AppsDatastoreUpdate;
query: AppsDatastoreQuery;
count: AppsDatastoreCount;
delete: AppsDatastoreDelete;
bulkDelete: AppsDatastoreBulkDelete;
};
Expand Down
1 change: 1 addition & 0 deletions src/typed-method-types/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const methodsWithCustomTypes = [
"apps.datastore.bulkPut",
"apps.datastore.update",
"apps.datastore.query",
"apps.datastore.count",
"apps.auth.external.get",
"apps.auth.external.delete",
"chat.postMessage",
Expand Down
1 change: 1 addition & 0 deletions src/typed-method-types/typed-method-tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Deno.test("Custom Type Methods are valid functions", () => {
assertEquals(typeof client.apps.datastore.bulkPut, "function");
assertEquals(typeof client.apps.datastore.update, "function");
assertEquals(typeof client.apps.datastore.query, "function");
assertEquals(typeof client.apps.datastore.count, "function");
assertEquals(typeof client.apps.auth.external.get, "function");
assertEquals(typeof client.apps.auth.external.delete, "function");
assertEquals(typeof client.workflows.triggers.create, "function");
Expand Down
Loading