-
Notifications
You must be signed in to change notification settings - Fork 883
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
Add msgpack obj init for complex types #1135
Add msgpack obj init for complex types #1135
Conversation
Signed-off-by: Athish Pranav D <[email protected]>
Signed-off-by: Athish Pranav D <[email protected]>
@redboltz Could you please check this PR |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## c_master #1135 +/- ##
============================================
- Coverage 55.45% 54.26% -1.20%
============================================
Files 8 8
Lines 1044 1067 +23
============================================
Hits 579 579
- Misses 465 488 +23 |
@redboltz just to give more context, we are trying to initialize the msgpack_obj and since its dynamically typed it needs hardcoded assigning. But since we can see 4 of the types have the same format (size and pointer) it will be more modular to have a function to do the same. I am trying to use this for one of the enhancements in |
@Athishpranav2003 In addition, tests for added function should be added the end of |
I will add them now. I forgot to add them in the hurry |
@redboltz i am having issues with executing the tests tho.
I tried exporting the |
The path Build and test should be at your working directory. Try this at you working directory,
|
Not sure whats the problem NVM. Running the test binary manually works. Thanks for helping me out |
@redboltz Added the UTs for the same. Please check if any more tests needs to be added |
d5be04a
to
9628c08
Compare
src/objectc.c
Outdated
return 0; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return 0; | |
return true; |
Please remove empty line here.
9628c08
to
48299a3
Compare
@redboltz addressed your comments |
48299a3
to
c997b8d
Compare
@redboltz addressed your comments |
Signed-off-by: Athish Pranav D <[email protected]>
c997b8d
to
985184b
Compare
@redboltz sorry for force pushing the changes. I just moved the assigning part to end of function and removed the nil line |
I noticed another issues about the PR. So I created #1137, which includes your contribution. Here’s the background: I considered why msgpack_object_init() returns a boolean and under what conditions it could fail. It seems that the failure occurs when an invalid type parameter is passed. I also thought about whether msgpack_object_init() could be extended to support all msgpack object types in the future. However, I realized that doing so while maintaining a clean interface would be difficult. From this, I came to two conclusions. First, the name msgpack_object_init() is too broad, covering various meanings (e.g., nil, boolean, int, etc.). Second, if the function name included the type, the type parameter could be eliminated, and failures would no longer occur. Please review #1137 to see if it addresses your issue. |
This PR addresses the feature request to initialise msgpack objects such as string, bin, array and map
Closes #1134