-
Notifications
You must be signed in to change notification settings - Fork 48
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
Implement a delete
method and state on the @web5/api
Record
class
#635
Conversation
🦋 Changeset detectedLatest commit: f8469f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
TBDocs Report ✅ No errors or warnings @web5/api
@web5/crypto
@web5/crypto-aws-kms
@web5/dids
@web5/credentials
TBDocs Report Updated at 2024-05-30T13:38:18Z |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #635 +/- ##
==========================================
+ Coverage 90.81% 90.92% +0.11%
==========================================
Files 119 119
Lines 30101 30258 +157
Branches 2243 2276 +33
==========================================
+ Hits 27337 27513 +176
+ Misses 2729 2710 -19
Partials 35 35
|
1f4d6f8
to
eca7c8a
Compare
8af3134
to
373e883
Compare
373e883
to
4a417fc
Compare
7730d23
to
28c5d8f
Compare
7da6f8a
to
46375cb
Compare
delete
method and state on the @web5/api
Record
classdelete
method and state on the @web5/api
Record
class
@LiranCohen worth noting that this exact functionality was removed in a prior PR: Relevant section:
It's been a while but @csuwildcat, @angiejones , and @blackgirlbytes might recall some other context of the motivation for removing this. |
RE the previous "foot gun", we now account for the I was thinking in a subsequent PR to remove the Are there any cases where you see it valid to be able to issue a delete while ONLY knowing the Worth noting that this functionality is useful for the upcoming Additionally there have been discussions of allowing |
@frankhinek I spoke to both @blackgirlbytes and @csuwildcat regarding this feature and I think we are all on the same page. @csuwildcat suggested we keep both of the Was there anything else with the implementation itself that needed attention? |
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.
🚀 Ship it!
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.
Rubber stamp. 🚀
This PR Allows a
Record
class to include adeleted
state based on aRecordsDeleteMessage
being applied to it.We update the
RecordModel
to reflect the immutable properties that a record may contain, as well as the optional mutable properties that are only relevant to a record that is in a non-deleted state.When a record is in a deleted state, it derives the immutable properties from the
initialWrite
of that record.Currently when dealing with a remote DWN, the user cannot be notified of a deleted state through a Query, however when Subscriptions are brought up to the API a user could receive a message from the remote which signals that the record has been deleted, more useful scenario testing will be implemented when
record.subscribe()
feature is added.delete()
method on theRecord
class that behaves similarly toupdate()
.deleted
property on theRecord
class to indicate if the record is in a deleted state.