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

Add MsgPack Marshal and Unmarshal function(Add Test Case And Change return value to unnamed) #263

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

Conversation

piyongcai
Copy link

@piyongcai piyongcai commented Nov 23, 2021

Note: Marshal max digits is 31. it decimal.IntPart large than 31,it will be lose extra digits.

Mote: Marshal max digits is 30. it decimal.IntPart large than 30,it will be lose extra digits.
Copy link
Member

@mwoss mwoss left a comment

Choose a reason for hiding this comment

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

Before diving into the implementation I would like to see a few unit tests. They are handy in case of any refactors, bug catching and implementation changes that could happened in the future

}

// UnmarshalMsg implements msgp.Unmarshaler
func (d *Decimal) UnmarshalMsg(b []byte) (o []byte, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you stick to one type of return value (names/unnamed)? Across the entire library, we use an unnamed one, so I personally opt for this approach.

Copy link
Author

Choose a reason for hiding this comment

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

already Change MsgPack MarshalMsg and UnmarshalMsg function's return value to unnamed.

@piyongcai piyongcai changed the title Add MsgPack Marshal and Unmarshal function Add MsgPack Marshal and Unmarshal function(Add Test Case And Change return value to unnamed) Dec 6, 2021
@piyongcai
Copy link
Author

Before diving into the implementation I would like to see a few unit tests. They are handy in case of any refactors, bug catching and implementation changes that could happened in the future

Test Case was added.

@piyongcai piyongcai requested a review from mwoss December 6, 2021 06:10
@mwoss
Copy link
Member

mwoss commented Dec 7, 2021

Thanks for providing test cases @piyongcai :) I will try to review your code later this week or next to one, as I need to get familiarized with MsgPack before doing a proper review.

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