-
-
Notifications
You must be signed in to change notification settings - Fork 159
Added data source endpoints and refactored database endpoints #286
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
Conversation
- Refactored `DatabasesEndpoint` methods (note: no pytest tests included)
…`notion-sdk-js`: in `DataSourcesEndpoint.update`, added `"parent"` to the `body` parameter
…538` from `notion-sdk-js`: added support for `is_locked` in update methods of page and database APIs
…additional_data` (see makenotion/notion-sdk-js#603)
- Updated `tests\cassettes` to change `notion-version: - '2025-09-03'` - Modified `tests\conftest.py` for format compatibility
- Added `"is_locked"` to the `body` of the `update` function in `DatabasesEndpoint`
- Hardcoded `page_id`
- Normalize code formatting
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 367 375 +8
=========================================
+ Hits 367 375 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Could I Remove support for Python 3.7? |
- Improved tests for `request_id` and `additional_data` - Enhanced `_parse_response` to handle `additional_data` and `request_id` - Improved initialization tests for `APIResponseError`
|
Hey @tsinglinrain, thanks for your work here! From a (very) quick look, it seems to go in the right direction, but please try to keep the PR as thin as possible so that it's easier to review. I'd rather review multiple small PRs than one big PR. For example, I don't think I'd approve adding more debug logs. But on the other side, you removed For Python 3.7, I'm most likely okay to remove it, but why do we want that? Also, please remove Chinese comments. :) |
|
@ramnes Thank you for your feedback. Sorry about this, but please don’t review it just yet — I plan to simplify this PR and keep only the data source part, and then you can take another look afterward. (I intend to split it into two PRs.) I merged some changes along the way because they felt convenient to include, but I realize now that I didn’t fully understand the importance of keeping branches focused — apologies for that. The debug logging changes were added because I noticed in TypeScript we have this: this.log(LogLevel.INFO, "request success", {
method,
path,
...("request_id" in responseJson && responseJson.request_id
? { requestId: responseJson.request_id }
: {}),
})So I went ahead and added debug logs for this part as well. Since vcrpy has been upgraded to 6.0.2, which no longer supports Python 3.7, I’d also like to ask if it would be possible to drop Python 3.7 support. (To make the tests pass, I updated |
No worries, looking forward!
Oh, interesting. Happy to review a PR that makes logging in this library identical to notion-sdk-js logging.
Makes sense! You can remove it. |
|
@ramnes what is missing here to get this merged & published in the package? |
- Removed mentions of Python 3.7 support from `README.md` and other docs - Updated minimum supported Python version to 3.8
|
@ramnes I have simplified this PR to keep only the data source and database parts. Could you please review it? |
|
|
- Set `"decode_compressed_response"` back to `True` to ensure complete content in `tests\cassettes` - Fixed typo: renamed `test_file_uploads_list_with_stat_cursor` to `test_file_uploads_list_with_start_cursor`
|
Any updates on this? |
@FlorianWilhelm No more updates, |
|
I'll review this week! |
…est code in `tests\test_endpoints.py`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just a quick note for future PRs if you plan on opening more: please avoid any diff that's not strictly related to what you're doing (e.g. reformatting unrelated strings, docs, etc.) It makes the diff harder to read and will clutter the git history.
Thanks for the great work!
|
@ramnes Thanks a lot for the review and the tip! |
Added
data sourceendpoints and refactoreddatabaseendpointsDataSourcesEndpointclass, the official documentation is inconsistent in casing, using bothData Sourceanddata source. I chose to follow the official documentation exactly as written.DataSourcesEndpointclass, the order ofBody Paramsfollows the official JS SDK rather than the API reference documentation.Added support for
is_lockedin pageupdate.Added
additional_datain error responses.(Note: This PR mainly focuses on data source and database updates, and does not fully cover all changes introduced in the latest TS version.)
tests\cassettesused an oldnotion-version, I replaced them in bulk. However, the new version tests failed becausedecode_compressed_response: Truecaused VCR to attempt decompression, expecting abody.stringformat, while the new format usescontent. Setting it toFalseresolved the issue.vcrpy 5.1.0 + httpx 0.28.1produces an unstable format mix, whilevcrpy 6.0.2 + httpx 0.28.1consistently outputs thebody.stringformat. Therefore, I upgradedvcrpyto version 6.0.2 (keepinghttpx 0.28.1), and removed the previously written compatibility function. With this change, httpx automatically handles compression, so I setdecode_compressed_response: False, ensuring cassette files retain their raw state — making debugging easier and reducing potential VCR-related bugs.tests\cassetteswere regenerated, except fortests\test_helpers.py.re.sub(RE_PAGE_ID, r1.uri, "")was likely incorrect. Inre.sub(pattern, repl, string), the second parameter (repl) should be the replacement text, not the input string. I fixed this with a small change to improve robustness and error handling.related:#275