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

Fix some type defs #5

Merged
merged 6 commits into from
Jan 8, 2025
Merged

Conversation

mwildehahn
Copy link
Contributor

I should have done this as a couple commits but the changes are pretty minimal:

  • I fixed some of the getter/setter methods that were generated
  • I added ruff as a dev requirement to format the file

@mwildehahn
Copy link
Contributor Author

Add a type guard to make parsing values / containers from the maps easier.

It still feels a little verbose but better than having to do:

value_type = meta.get("key")
assert value_type is not None
assert isinstance(value_type, ValueOrContainer.Value)
value_type.value

now you can just do:

value_type = meta.get("key")
assert ValueOrContainer.is_value(value_type)
value_type.value

@mwildehahn
Copy link
Contributor Author

mwildehahn commented Jan 7, 2025

The enum values work at runtime but fail against a linter (pylance) for me. Looking into it: https://docs.python.org/3/howto/enum.html enums are supposed to map to unique static values.

This is the error I see in my project:

Object of type "Literal[TreeParentId.Node]" is not callable
  Attribute "__call__" is unknown

I think instead of these, it'd be better to use something like TypedDict, we could then do something like:

class TreeParentRoot(TypedDict):
    type: Literal["root"]


class TreeParentNode(TypedDict):
    node: TreeID


class TreeParentDeleted(TypedDict):
    type: Literal["deleted"]


class TreeParentUnexist(TypedDict):
    type: Literal["unexist"]


TreeParentId = TreeParentRoot | TreeParentNode | TreeParentDeleted | TreeParentUnexist

tree.children({"type": "root"})
tree.children({"node": node_id})

If we wanted to still enable the factory method, we could do something like:

class TreeParent:
   def Node(node_id: TreeID) -> TreeParentNode:
      return {"node": node_id}
    ...
 
 
 tree.children(TreeParent.Node(node_id))

@mwildehahn
Copy link
Contributor Author

After looking into this, I think the root of the issue was the stub using Enum: Jij-Inc/pyo3-stub-gen#119.

pyo3 just generates these as classes so we should just define them as such in the stubs. That fixed the errors for me!

The one thing that is a little weird about this to me now is to_delta returning a list of dicts vs apply_delta taking a list of TextDelta types.

Ideally we'd return the same thing here. For now at least you get typed dicts back when you invoke to_delta

@Leeeon233
Copy link
Member

Thanks for your contribution! These are issues I overlooked.

Regarding TreeParentId, I'm thinking of not exporting it, but rather using Optional[TreeID], which aligns with #4

You're right about keeping the types consistent between to_delta and apply_delta. I plan to merge this PR first and address these two issues in separate PRs.

@mwildehahn
Copy link
Contributor Author

Regarding TreeParentId, I'm thinking of not exporting it, but rather using Optional[TreeID], which aligns with #4

Yea, this would be great -- None would just assume root?

@mwildehahn
Copy link
Contributor Author

From working with the swift code, it would be nice to export TreeID so you can manually construct it. From my swift code, I had to use exportJsonUpdates, parse the JSON and then sometimes manually make TreeIDs from that so I could work with the tree again.

Ideally we could just exportUpdates without the JSON passthrough but it works for now.

@Leeeon233
Copy link
Member

Regarding TreeParentId, I'm thinking of not exporting it, but rather using Optional[TreeID], which aligns with #4

Yea, this would be great -- None would just assume root?

Yes

@Leeeon233 Leeeon233 merged commit 4cdf4c3 into loro-dev:main Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants