8000 Updated YMap and YArray API by Waidhoferj · Pull Request #49 · y-crdt/ypy · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Apr 22, 2025. It is now read-only.

Updated YMap and YArray API #49

Merged
merged 2 commits into from
May 7, 2022
Merged

Updated YMap and YArray API #49

merged 2 commits into from
May 7, 2022

Conversation

Waidhoferj
Copy link
Collaborator

API Updates

  • YMap.delete -> YMap.pop: Reflects the dict api. Pop returns the removed value and throws a key error if it doesn't find a value.
  • Added YMap.get: gets value from array with optional default value.
  • YArray.delete -> YArray.delete_range
  • YArray.insert -> YArray.insert_range: Now takes any iterable
  • Added YArray.insert: Inserts a single element at index
  • Added YArray.append: adds value to the end of the list
  • Added YArray.delete: Deletes individual value

Internel updates

  • py_into_any refactored with more explicit error handling. Instead of returning None on error conditions, it now returns either a MultipleIntegrationError when attempting to integrate a non preliminary value and a TypeError when the supplied type cannot be integrated into YCRDT as Any. This centralizes the definition of errors and resolves issues in Better Integration Error Messages #46

@Waidhoferj Waidhoferj added the enhancement New feature or request label May 6, 2022
@Waidhoferj Waidhoferj self-assigned this May 6, 2022
@Waidhoferj Waidhoferj mentioned this pull request May 6, 2022
Closed
@dmonad
Copy link
Contributor
dmonad commented May 6, 2022
  • add del map[“key”]
  • del l[3:] instead of delete_range

@Waidhoferj
Copy link
Collaborator Author
Waidhoferj commented May 6, 2022 •
edited
Loading
  • add del map[“key”]
  • del l[3:] instead of delete_range

Just realized we have the same problem with this approach as insertions: deleting an element requires a transaction. To support this behavior, we would need to pass along the transaction del map[(txn, "key")] which is less discoverable than delete(txn, key) which has accessible type annotations.
@dmonad

@davidbrochart
Copy link
Collaborator

Then let's keep only pop. Note that pop accepts a default value.

@Waidhoferj
Copy link
Collaborator Author

Sounds good. Thanks for the heads up, I missed the default value.

@Waidhoferj
Copy link
Collaborator Author

@davidbrochart Yrs doesn't expose the ability to return a value from Array.remove(index), which is why I did not implement YArray.pop(index). We could approach this a few ways:

  1. Keep the delete functions, since they match current behavior.
  2. Implement a pop that doesn't return anything with a promise to change it in the future.
  3. Make a monkypatch version of pop that matches Python, but has to traverse the array twice: once to get the value and again to delete it.

I'm partial to the first, but what do you all think?

@davidbrochart
Copy link
Collaborator

Yes, keeping only delete is fine.

Waidhoferj added 2 commits May 6, 2022 09:08
@Waidhoferj
Updated YMap and YArray API:
2eaba07 
- YMap.delete -> YMap.pop: Reflects the dict api. Pop returns the removed value and throws a key error if it doesn't find a value.
- Added YMap.get: gets value from array with optional default value.
- YArray.delete -> YArray.delete_range
- YArray.insert -> YArray.insert_range: Now takes any iterable
- Added YArray.insert: Inserts a single element at index
- Added YArray.append: adds value to the end of the list
- Added YArray.delete: Deletes individual value
@Waidhoferj
consolidated errors, added default to YMap.pop
4a22f9b
@Waidhoferj Waidhoferj force-pushed the map-array-api-updates branch from ccffc72 to 4a22f9b Compare May 6, 2022 16:53
@Waidhoferj
Copy link
Collaborator Author

@dmonad Rebased and made changes, ready for review.

dmonad
dmonad approved these changes May 7, 2022
View reviewed changes
Copy link
Contributor
@dmonad dmonad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@Waidhoferj Waidhoferj merged commit 3351404 into main May 7, 2022
@Waidhoferj Waidhoferj deleted the map-array-api-updates branch May 7, 2022 21:54
@davidbrochart davidbrochart mentioned this pull request May 9, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dmonad dmonad dmonad approved these changes

@Horusiath Horusiath Awaiting requested review from Horusiath

@davidbrochart davidbrochart Awaiting requested review from davidbrochart

Assignees

@Waidhoferj Waidhoferj

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Waidhoferj @dmonad @davidbrochart
0