-
Notifications
You must be signed in to change notification settings - Fork 91
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
Use our own json handle for decoding #765
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #765 +/- ##
===========================================
+ Coverage 55.17% 55.20% +0.02%
===========================================
Files 27 28 +1
Lines 3815 3817 +2
===========================================
+ Hits 2105 2107 +2
Misses 1435 1435
Partials 275 275
Continue to review full report at Codecov.
|
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.
Why move the jsonCodeHandle into a new encoding.go
file in the same package? Readability, or is there something obvious I am missing?
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.
I agree with the comments Brian made some time ago. I'd rather see encoding/decoding merged into a single file with all functions side-by-side. For me, separate encode/decode files make this code more difficult to read and review, further separation would make things even more difficult.
I really think it's not a problem, as you can have two windows side-by-side, but I put encoding, decoding functions in the same file. |
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.
LGTM
Summary
We already use our own json handle for encoding. For consistency and to guard against changes in go-algorand, use the same json handle for decoding.