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

Add update database to client #170

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Add update database to client #170

merged 4 commits into from
Aug 12, 2021

Conversation

kiranpandit
Copy link
Contributor

Adding support for update database: https://developers.notion.com/reference/update-a-database

@kiranpandit kiranpandit requested a review from almawang August 12, 2021 20:21
extends DatabasesUpdatePathParameters,
DatabasesUpdateQueryParameters,
DatabasesUpdateBodyParameters {}
export interface DatabasesUpdateResponse extends BlockBase {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns a Database object right? not BlockBase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed!

Copy link
Contributor

@almawang almawang left a comment

Choose a reason for hiding this comment

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

I think the Response type needs to be updated and remove the non updatable property schema types

@@ -1040,3 +1040,28 @@ export interface LastEditedTimePropertySchema {
export interface LastEditedByPropertySchema {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not part of the PR, but why is this a PropertySchema? this shouldn't be editable right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for | CreatedTimePropertySchema | CreatedByPropertySchema | LastEditedTimePropertySchema | LastEditedByPropertySchema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these can be added to the database schema as properties, the values themselves are not updated

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see, sg!

src/api-types.ts Outdated
}

export interface UpdateMultiSelectPropertySchema {
multi_select: { options?: UpdateMultiSelectOptionSchema[] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have a separate type for the mulitselect options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point - updated so both use the same type

@kiranpandit kiranpandit requested a review from almawang August 12, 2021 21:54
type UpdateSelectOptionSchema = SelectOptionSchema | SelectOption

export interface UpdateSelectPropertySchema {
select: { options?: UpdateSelectOptionSchema[] }
Copy link
Contributor

Choose a reason for hiding this comment

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

the options field is nullable? what's the behavior when an empty object is passed into select?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it's undefined, then there will be no options (similar to when you create a "select" property in the client before adding any options). In create database, the options are optional (ha).

Copy link
Contributor

@almawang almawang left a comment

Choose a reason for hiding this comment

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

LGTM, question about the behavior of an empty select field

@kiranpandit kiranpandit merged commit 744d512 into main Aug 12, 2021
@kiranpandit kiranpandit deleted the kiran-update-db branch August 12, 2021 23:07
rhart92 pushed a commit that referenced this pull request Oct 3, 2023
* Add update database to client

* Fix typo

* Change response to Database

* Use the same type for both select and multi select options
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