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

Allow to set the in_trash property of a page using notion.pages.update() #233

Closed
jeromegit opened this issue May 12, 2024 · 7 comments
Closed
Labels
good first issue Good for newcomers help wanted Extra attention is needed task Something that has to be done at some point

Comments

@jeromegit
Copy link

Hey @ramnes,
Thanks for the great module!

Adding in_trash to the list in the line referenced below should do the trick:

body=pick(kwargs, "archived", "properties", "icon", "cover"),

Merci !

@jeromegit jeromegit changed the title Allow to set the in_trash property of a page using update() Allow to set the in_trash property of a page using notion.pages.update() May 12, 2024
@ramnes
Copy link
Owner

ramnes commented May 17, 2024

Hey @jeromegit, thanks for opening the issue!

Indeed, we should add in_trash. Rlated changelog from Notion: https://developers.notion.com/page/changelog#changes-for-april-2024

PR welcome!

@ramnes ramnes added help wanted Extra attention is needed task Something that has to be done at some point labels May 17, 2024
@ramnes ramnes added the good first issue Good for newcomers label Jun 11, 2024
@digzect
Copy link
Contributor

digzect commented Jul 12, 2024

Hey @jeromegit @ramnes , thanks for your comments on this issue!
I've changed archived to in_trash in notion_client/api_endpoints.py

https://github.com/digzect/notion-sdk-py/blob/6b1eb8f9972bb851fc6585aa80a184b397bfd2d5/notion_client/api_endpoints.py#L235 , and the pytest coverage is now 100%.
Am I ready to create a PR at this point? This is my first time contributing, so I would appreciate your guidance.

@ramnes
Copy link
Owner

ramnes commented Jul 12, 2024

Why removing archived? The changelog entry above says that both are still supported. Did it get removed in the meantime?

Otherwise yes, just push your commit to a branch on your repository and open a PR!

@digzect
Copy link
Contributor

digzect commented Jul 13, 2024

Thank you for your response.
I've reviewed the Notion API documentation:

The commit in question is specifically for the PagesEndpoint, which is why I changed archived to in_trash.
Additionally, When using both archived and in_trash together in the code, the pytest coverage remains at 100%.

Given this information, how would you suggest we proceed? Should we keep only in_trash for the PagesEndpoint, or include both for maximum compatibility?

@ramnes
Copy link
Owner

ramnes commented Jul 13, 2024

I'd say let's take a look at notion-sdk-js and do whatever the Notion developers did there. :)

@digzect
Copy link
Contributor

digzect commented Jul 13, 2024

I've looked at notion-sdk-js and using both "archived" and "in_trash" together.
https://github.com/makenotion/notion-sdk-js/blob/7950edc034d3007b0612b80d3f424baef89746d9/src/api-endpoints.ts#L10514
Thank you for your guidance!

@ramnes
Copy link
Owner

ramnes commented Jul 13, 2024

Fixed in #236. Thanks!

@ramnes ramnes closed this as completed Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed task Something that has to be done at some point
Projects
None yet
Development

No branches or pull requests

3 participants