refactor: move data conversion code to internal/converters #473
Conversation
t0mk
force-pushed
the
move_more_utils_to_internal
branch
from
6850679
to
33f7117
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
- Coverage 60.11% 59.64% -0.48%
==========================================
Files 99 95 -4
Lines 20045 19752 -293
==========================================
- Hits 12051 11782 -269
+ Misses 7691 7679 -12
+ Partials 303 291 -12 ☔ View full report in Codecov by Sentry. |
internal/converters/hashcode.go
Outdated
| @@ -11,7 +11,7 @@ import ( | |||
| // and invert it if the result is negative. | |||
| // | |||
| // Originally from https://github.com/hashicorp/terraform-plugin-sdk/blob/main/internal/helper/hashcode/hashcode.go | |||
| func hashcodeString(s string) int { | |||
| func HashcodeString(s string) int { | |||
There was a problem hiding this comment.
We could move this to a hash or hashcode package and rename to String
internal/converters/converters.go
Outdated
|
|
||
| import ( | ||
| "strconv" | ||
| "strings" | ||
| ) | ||
|
|
||
| func contains(s []string, e string) bool { | ||
| func Contains(s []string, e string) bool { |
There was a problem hiding this comment.
If we build on go 1.21 instead of 1.20 we could replace this with usage of the slices standard package (even if we don't upgrade, we could use x/exp/slices.
There was a problem hiding this comment.
Replacement of this function could be saved for a later change, but if we opt to keep this implementation of Contains, I think it would fit better in a package named slices, since it isn't converting anything.
internal/converters/converters.go
Outdated
| var arr []interface{} | ||
| for _, v := range sli { | ||
| arr = append(arr, v) | ||
| } | ||
| return arr | ||
| } | ||
|
|
||
| func convertStringArr(ifaceArr []interface{}) []string { | ||
| func ConvertStringArr(ifaceArr []interface{}) []string { |
There was a problem hiding this comment.
Let's take this opportunity to improve these function names. StringArrToIfArr is a nice, descriptive name, and if we follow that model here, this function would be called IfArrtoStringArr
internal/converters/converters.go
Outdated
| @@ -33,7 +33,7 @@ func convertStringArr(ifaceArr []interface{}) []string { | |||
| return arr | |||
| } | |||
|
|
|||
| func convertIntArr(ifaceArr []interface{}) []string { | |||
| func ConvertIntArr(ifaceArr []interface{}) []string { | |||
There was a problem hiding this comment.
This would be IfArrToIntStringArr (or can it be safely replaced with usage of other converters?)
internal/converters/converters.go
Outdated
| @@ -44,7 +44,7 @@ func convertIntArr(ifaceArr []interface{}) []string { | |||
| return arr | |||
| } | |||
|
|
|||
| func convertIntArr2(ifaceArr []interface{}) []int { | |||
| func ConvertIntArr2(ifaceArr []interface{}) []int { | |||
There was a problem hiding this comment.
This would be IfArrToIntArr
t0mk
force-pushed
the
move_more_utils_to_internal
branch
from
33f7117
to
66ff19d
Compare
t0mk
force-pushed
the
move_more_utils_to_internal
branch
from
66ff19d
to
9a838bb
Compare
|
@ctreatma I fixed your review comments. |
|
@ctreatma I remove hashcode.go from |
|
@t0mk could you push up that change? GitHub still shows the old commit with both hashcode copies. |
t0mk
force-pushed
the
move_more_utils_to_internal
branch
from
9a838bb
to
a2002f4
Compare
t0mk
force-pushed
the
move_more_utils_to_internal
branch
from
a2002f4
to
21c2507
Compare
|
@ctreatma , sorry, I pushed it now. It should be ready for review again. |
This PR is part of a refactoring effort (#106). It moves some helper code to another internal subpackage.