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: implements encoding of floating point numbers as float64 #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jacaetevha
Copy link

I ported the the encoding of floats to float64 from https://github.com/feross/ieee754

@jacaetevha
Copy link
Author

This fails for negative doubles. Will fix that soon.

@patriksimek
Copy link
Owner

Hey, thanks for your contribution! I know it may sound weird, but may I ask you to avoid using helper functions and move everything into the msgpack_encode function? I don't like the idea of polluting the public namespace with helper functions when that's not necessary.

One more thing - can you please retain the old code formatting? Thanks!

@jacaetevha
Copy link
Author

I can reformat the code to use tabs instead of spaces.

Would you be more comfortable with helper functions that are all msgpack_* instead (eg. msgpack_array, msgpack_boolean, msgpack_float, msgpack_integer, msgpack_object, msgpack_text)? I could certainly inline the smaller helper functions (is_nan and is_negative_zero). float_to_bytea would be renamed to msgpack_float.

@jacaetevha
Copy link
Author

@patriksimek any thoughts on having msgpack* functions?

@patriksimek
Copy link
Owner

I don't see much value in splitting that into multiple functions. There's no code duplication and those helper functions itself have no value to anyone. That being said, I'd still prefer to have everything in one function. Thanks!

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