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

Make sure nullable attributes icon and cover are set to None #220

Closed
wants to merge 2 commits into from

Conversation

FlorianWilhelm
Copy link
Contributor

This fixes #219.

It would be really nice if this could be merged and released so that I can also make a new release of Ultimate Notion. Thank you 🙏

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (bc03f56) 100.00% compared to head (41f646b) 99.67%.

❗ Current head 41f646b differs from pull request most recent head 7a8331f. Consider uploading reports for the commit 7a8331f to get more accurate results

Files Patch % Lines
notion_client/api_endpoints.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #220      +/-   ##
===========================================
- Coverage   100.00%   99.67%   -0.33%     
===========================================
  Files            7        7              
  Lines          300      304       +4     
===========================================
+ Hits           300      303       +3     
- Misses           0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramnes ramnes closed this in 28020cd Dec 25, 2023
@ramnes
Copy link
Owner

ramnes commented Dec 25, 2023

Thanks for the PR @FlorianWilhelm! I went with a bit more extreme approach, can you tell me if the latest commit works for you? See the rationale in #101.

@ramnes
Copy link
Owner

ramnes commented Dec 25, 2023

PS – Regarding Ultimate Notion, I'm always glad to see some effort to build a higher-level library on top of notion-sdk-py so thank you for this! Did you check notional? It's been there for a while now and seems to be willing to achieve the same goals as you are and would probably benefit from more contributors. :)

Also, if you see things that are missing from notion-sdk-py but do exist in notion-sdk-js (e.g. methods type hints), please consider contributing here rather than downstream!

@FlorianWilhelm FlorianWilhelm deleted the fix-del-icon-cover branch December 26, 2023 15:00
@FlorianWilhelm
Copy link
Contributor Author

@ramnes Thanks for fixing this issue and I think your approach is even nicer than my PR.

Regarding notional, I tried to use it, but it doesn't seem to be under active development anymore, and I don't agree with many of the design decision. This is why I copied some parts over as I like the Pydantic approach, but added an extra layer of indirection, which famously solves almost all problems in computer science 😅. I also think that Ultimate Notion is much easier to use compared to Notional and of course more pythonic. But doesn't every developer say that about his/her own software? ;-)

I'll definitely contribute to notion-sdk-py whenever I find an issue, as your package is the base for my package. Sadly, I also found a lot of bugs within the Notion API itself, but the Notion devs are really not that responsive. For instance, whenever I try to make a self-referential two-way relation, for instance to express a parent task and child tasks in a task database, the Notion API only returns a one-way relation. Creating a two-way relation from a database A to a database B works perfectly but only if A is not B.
What are your experiences with the Notion devs and bugs in their API or is it just me 😅 ?

@ramnes
Copy link
Owner

ramnes commented Dec 26, 2023

Got to admit that I haven't talked to them much so far. Did you try the Notion API developers Slack?

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.

Deleting an icon or a cover is not possible.
2 participants