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

Passthrough int and tuple #64

Merged
merged 1 commit into from
Jan 8, 2022
Merged

Passthrough int and tuple #64

merged 1 commit into from
Jan 8, 2022

Conversation

TariqTNO
Copy link
Contributor

@TariqTNO TariqTNO commented Jan 7, 2022

We have developped some functionality for ormsgpack that is currently not supported, but you might want to support:

This PR adds:

  • Optional passthrough of Python integers, such that also ints larger than 64 bits can be serialized using a default function. Currently they can not be serialized in any way, and by adding this option, the library doesn't get more complicated, but serialization of larger integers is allowed. (Updated tests accordingly)
  • Optional passthrough of Python tuples, such that they can also be deserialized as tuples. Again, no complications for the library, but just leaving it to the default option. (Updated tests accordingly)
  • An OPT_NO_OPT constant, for convenience of the user (with value 0).
  • Fixed typing of the option parameter, since in fact only ints are allowed, and None-values give errors.

Please let me know if you would like to add (part of) this functionality.

@aviramha
Copy link
Owner

aviramha commented Jan 7, 2022

Thanks for your contribution!

  1. I'd alter the opt so the pass through is for integers that exceed that size. This way, if you have integers that are less than 64 bit you'll still serialize faster (avoid calling into Python) and it makes a bit more sense.
  2. Instead of using the passthrough functionality, you can change the behavior of when serializing tuples when the flag is on. For same reason as above (speed) and simplicity.
  3. You can just call the function without passing any argument to the opt..
  4. Sounds like a bug. I'll open a ticket to fix it later on. Can't pass None to option parameter #65

@aviramha
Copy link
Owner

aviramha commented Jan 7, 2022

Nevermind regarding the tuple as we really can't distinguish those in msgpack spec.

@ThomasTNO
Copy link
Contributor

On 3., when you want to pass-through option variable to an end-user having a variable representing 0 is more explicit.

e.g.

def pack_this_for_me_please(obj, option = ormsgpack.OPT_PASSTHROUGH_INT):
    <some preprocessing>
    return ormsgpack.packb(value, option=option)

If one now wishes to disable OPT_PASSTHROUGH_INT it has to set the default variable of pack_this_for_me_please, option=ormsgpack.OPT_PASSTHROUGH_INT, to 0.

If #65 is resolved, one can just pass None and there is no use in having an OPT_NO_OPT

@aviramha
Copy link
Owner

aviramha commented Jan 7, 2022

On 3., when you want to pass-through option variable to an end-user having a variable representing 0 is more explicit.

e.g.

def pack_this_for_me_please(obj, option = ormsgpack.OPT_PASSTHROUGH_INT):

    <some preprocessing>

    return ormsgpack.packb(value, option=option)

If one now wishes to disable OPT_PASSTHROUGH_INT it has to set the default variable of pack_this_for_me_please, option=ormsgpack.OPT_PASSTHROUGH_INT, to 0.

If #65 is resolved, one can just pass None and there is no use in having an OPT_NO_OPT

Let's fix it instead of making work around 😅.
I'll try to get to it this weekend. If you want you can submit a patch.

@aviramha
Copy link
Owner

aviramha commented Jan 7, 2022

I fixed the opt=None thingy. Besides my other notes, make sure to add a CHANGELOG entry.

@TariqTNO
Copy link
Contributor Author

TariqTNO commented Jan 8, 2022

Tnx for the quick feedback and resolving of #65. I'll process your notes today and a CHANGELOG entry.

@TariqTNO
Copy link
Contributor Author

TariqTNO commented Jan 8, 2022

@aviramha

  1. I've followed your suggestion and am now only passing through integer that do not fit an i64. I tried to also check for fitting in a u64, but then one needs to determine the sign and there was no easy way of doing that. Also I felt it was not worth it to postpone that check to the (U)IntSerializer as that requires making those structs larger, leading to a performance decrease. Since going out of i64 but staying inside u64 is not likely, I think this is the best way.
  2. Kept it as it was.
  3. Removed it, since no longer necessary.
  4. Also reverted this, since you already fixed it.

I've also added a CHANGELOG note.

Copy link
Owner

@aviramha aviramha left a comment

Choose a reason for hiding this comment

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

Add credit in the changelog and I'll merge it :)

CHANGELOG.md Outdated Show resolved Hide resolved
@aviramha aviramha merged commit ad13a12 into aviramha:master Jan 8, 2022
@aviramha
Copy link
Owner

aviramha commented Jan 8, 2022

Thanks a lot. I'll release 1.1.0 today.

@TariqTNO TariqTNO deleted the passthrough_int_and_tuple branch January 10, 2022 07:06
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.

3 participants