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

feat: add wrapper types for all integer/float (de)serialization #322

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Sep 17, 2024

No description provided.

@wyfo wyfo requested a review from Mallets September 17, 2024 11:43
Copy link
Contributor

PR missing one of the required labels: {'documentation', 'internal', 'dependencies', 'enhancement', 'breaking-change', 'bug', 'new feature'}

@wyfo wyfo added the enhancement Things could work better label Sep 17, 2024
@wyfo
Copy link
Contributor Author

wyfo commented Sep 17, 2024

All Int8/UInt8/etc. has been added.
The question is now: should we keep support for int and float? In this case, it must be clearly documented than int would be equivalent to Int64 (arbitrary choice), and float to Float64 (arbitrary choice).
Otherwise, the use can add the support with only 4 lines of Python:

serializer(lambda i: ZBytes.serialize(Int64(i)), target=int)
deserializer(lambda zbytes: zbytes.deserialize(Int64), target=int)
serializer(lambda f: ZBytes.serialize(Float64(f)), target=float)
deserializer(lambda zbytes: zbytes.deserialize(Float64), target=float)

@wyfo wyfo changed the title feat: add Float32 for f32 (de)serialization feat: add wrapper types for all integer/float (de)serialization Sep 17, 2024
@wyfo wyfo marked this pull request as ready for review September 17, 2024 16:11
@Mallets
Copy link
Member

Mallets commented Sep 17, 2024

I'd go for keeping int and float and mapping it to Int64 and Float64, which is kind of the same behaviour of usize on 64bit architecture on Rust.
It needs to be clearly documented.

@wyfo wyfo mentioned this pull request Sep 17, 2024
@wyfo
Copy link
Contributor Author

wyfo commented Sep 17, 2024

I've added ZBytes documentation. z_bytes.py example is in the PR following.

@@ -54,7 +90,7 @@ class Config:
def from_file(cls, path: str | Path) -> Self: ...
@classmethod
def from_json5(cls, obj: Any) -> Self: ...
@property
property
Copy link
Member

@Mallets Mallets Sep 18, 2024

Choose a reason for hiding this comment

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

Is @property -> property a wanted change or some simple error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a typo indeed, I'm fixing it

zenoh/__init__.pyi Outdated Show resolved Hide resolved
@Mallets Mallets merged commit 20b72ed into eclipse-zenoh:main Sep 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Things could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants