-
Notifications
You must be signed in to change notification settings - Fork 51
Feat/conv package: refactored conversions with generics #108
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
Conversation
Also: * refactored all the conversion and pointer/value methods as a separate package, thus deprecating their usage from the swag package. NOTE: * This is a first step towards the complete split of the vast API of the swag package into smaller packages, which will eventually become go modules, thus reducing the spread of dependencies (e.g. easyjson and yaml). Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #108 +/- ##
==========================================
- Coverage 89.03% 87.54% -1.50%
==========================================
Files 15 19 +4
Lines 1970 1678 -292
==========================================
- Hits 1754 1469 -285
+ Misses 183 176 -7
Partials 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code suggestion to simplify test Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>
|
@ccoVeille you may also find this read interesting, as I've tried to explain the (long-winded) migration plan that all these PRs try to contribute to: #68 |
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
| // NOTE: [unsafe.SizeOf] simply returns the size in bytes of the value. | ||
| // For primitive types T, the generic stencil is precompiled and this value | ||
| // is resolved at compile time, resulting in an immediate call to [strconv.ParseFloat]. | ||
| var v T | ||
| f, err := strconv.ParseFloat(str, int(unsafe.Sizeof(v))*8) |
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.
This is good, but you are using unsafe.Sizeof in various place here in this file
Please consider:
- creating a sizeof.go file
- add a sizeOfNumber func
- add this exact comment
- call sizeOfNumber everywhere else
This way, only sizeof.go will import the unsafe package and it would be clearer
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.
Did you see this before merging ?
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.
ok thanks. I'v been considering that wrapper func too. Perhaps in a follow-up. Now it is time to merge and move on to the next series of packages
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.
|
again a false negative from codecov: reducing the code automatically magnifies the few uncovered (pre-existing) parts of the code |
feat: add generics for conversion and pointer utilities