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

to_json compatibility with Python json #102

Merged
merged 1 commit into from
Dec 19, 2022
Merged

to_json compatibility with Python json #102

merged 1 commit into from
Dec 19, 2022

Conversation

Waidhoferj
Copy link
Collaborator

Updates to_json methods to return str types so that they can be used with Python json

Fixes #85

Changes

  • Separated type validation into CompatiblePyType, which validates incoming Python types to ensure they can be represented in the YCRDT.
  • Added JsonBuilder, which can append json from any JsonBuildable object to a string buffer.
  • Updated tests to cover to_json conversion.
  • Changed YArray iterator method to return a PyIterator on eagerly evaluated array contents.

Type Conversion Best Practices

In the past, we used to_json as a conversion to native Python types. In the future, we should use the direct constructor of the type we desire:

python_list = list(y_array)
python_dict = dict(y_map)
python_string = str(y_text)

Note that this is not a deep conversion.

YArray Iterator Update

This addresses an unknown bug associated with the move behavior. While move works correctly when we pull the Any representation directly from the Yrs Array, the iterator from the raw pointer seems to move with the moved element.

doc = YDoc()
    arr = doc.get_array('test')

    # Move 9 to 0
    with doc.begin_transaction() as t:
        arr.delete_range(t, 0, len(arr))
        arr.extend(t, [0,1,2,3,4,5,6,7,8,9])
    doc.transact(lambda t: arr.move_to(t, 9, 0))
    assert arr.to_json() == '[9,0,1,2,3,4,5,6,7,8]' # passes
    assert list(arr) == [9,0,1,2,3,4,5,6,7,8] # fails returning [9.0]

While this eager evaluation isn't ideal, it does match the expected behavior in y-wasm and fixes a significant bug with the move feature.

Updates `to_json` methods to return `str` types so that they can be used with Python `json`

## Changes
- Separated type validation into `CompatiblePyType`, which validates incoming Python types to ensure they can be represented in ycrdt.
- Added JsonBuilder, which can append json from any JsonBuildable object to a string buffer.
- Updated tests to cover to_json conversion.
- Changed YArray iterator method to return a PyIterator on eagerly evaluated array contents.
@Waidhoferj Waidhoferj added bug Something isn't working enhancement New feature or request labels Dec 19, 2022
@Waidhoferj Waidhoferj self-assigned this Dec 19, 2022
Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Thanks @Waidhoferj.
As a side note, could you make PRs from your fork of ypy in the future?

@Waidhoferj
Copy link
Collaborator Author

@davidbrochart Sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unintuitive .to_json() behavior
2 participants