-
Notifications
You must be signed in to change notification settings - Fork 56
237 - Add ability to clear date property #239
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
237 - Add ability to clear date property #239
Conversation
Updated c# from 7.3 to 8 for nullable reference type support
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.
@vijayvarmaD can we add integration tests to make sure it is working with Notion APIs? It doesn't have to be running automatically like other test cases but we should have a way to run them manually to make sure our SDK is working as expected after the changes.
Added the Integration Test. @KoditkarVedant, can you plz review the PR.
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.
I want to understand what is the reason behind adding nullable operator on a class?
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.
Hi @KoditkarVedant ,
As per the issue: #237 & the official APIs, Notion API allows us to clear the Date Property completely. Instead of individually making Start Date and EndDate as null.
As per my understanding, Date class is an exact map to the API Resource Value of Date , so making it as nullable was the only way to allow clearing the date. We cannot make DatePropertyValue nullable as we still need the property id and type.
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.
@vijayvarmaD I understand but Date is a class so it can be set to null. I am not sure why we need to user C# 8 and then add ? to class?
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.
Ok
@vijayvarmaD are you planning to re-open this?
Updated c# from 7.3 to 8 for nullable reference type support
Description
Add ability to clear date property
Made Date Property nullable in DatePropertyValue.cs
Nullable reference type is supported from C# 8.0, so bumped it.
Didn't make DatePropertyItem.cs date property nullable as it was just used for retrieval
Fixes # (issue)
#237
Type of change
#237 - Added Feature
How Has This Been Tested?
#237 - Tested the library by Updating the page of a database & also compared the request body sent by library with the official api postman collection.
Checklist: