chore: refactor nprintf #654
Conversation
Signed-off-by: Marques Johansson <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #654 +/- ##
===========================================
+ Coverage 37.91% 51.90% +13.99%
===========================================
Files 120 61 -59
Lines 19476 5040 -14436
===========================================
- Hits 7384 2616 -4768
+ Misses 11884 2224 -9660
+ Partials 208 200 -8 ☔ View full report in Codecov by Sentry. |
internal/sweep/sweep_test.go
Outdated
| "testing" | ||
|
|
||
| "github.com/equinix/terraform-provider-equinix/internal/resources/metal/vlan" |
There was a problem hiding this comment.
We should get something in place to enforce that imports are declared in the "correct" order, for some agreed-upon definition of correct, to avoid stylistic churn like this.
There was a problem hiding this comment.
Agreed. I stalled on getting this PR consistent because there were some import changes affected but the tools I used (golangci-lint, goimports) didn't have a strong enough opinion to update the existing import blocks. Updating beyond the files affected would be preferrable in another PR or at least a separate commit.
goimports wouldn't regroup all imports ignoring existing arbitrary white-space. Since it honors the existing gaps the desired 3 group import stanzas (built-in, external, internal) was not achieved.
| ) | ||
|
|
||
| // NPrintf is a helper function to replace placeholders in a string with values from a map | ||
| func NPrintf(format string, params map[string]interface{}) string { |
There was a problem hiding this comment.
IMO it would be preferable to leave this in equinix/ and deprecate it. While there is some reduction of repetition here compared to using fmt.Sprintf, the repetition has the benefit of being explicit and flexible.
There was a problem hiding this comment.
I think in this case I'd usually lean towards duplicating (not moving) nprintf into a separate package and marking the NPrintf function as deprecated in the same PR that moves the NE resources. Duplicating the function enables us to move only the NE resources without touching anything else, and doing that in one PR makes it clear that this is tech debt we're taking on to accomplish a specific goal.
Given our CODEOWNERS configuration, though, I could see having a separate PR to introduce the nprintf package; IMO a PR like that should only touch resources for which we are the owners, so we would introduce the nprintf package as a deprecated approach that exists only to facilitate code migration and update the ECX resources--without moving them, since they're going away soon--to demonstrate that nprintf package is a drop-in replacement for the private nprintf function. A follow-up PR could move the NE resources and it would be up to the NE team to review and adopt that PR.
Signed-off-by: Marques Johansson <[email protected]>
displague
force-pushed
the
refactor_nprintf
branch
from
e7708e9 to
e144ee0
Compare
| "strings" | ||
| ) | ||
|
|
||
| // NPrintf is a helper function to replace placeholders in a string with values from a map |
There was a problem hiding this comment.
| // NPrintf is a helper function to replace placeholders in a string with values from a map | |
| // NPrintf is a helper function to replace placeholders in a string with values from a map | |
| // | |
| // Deprecated: nprintf.NPrintf exists to support migrating resources and data source out | |
| // of the equinix package and into separate packages under `internal/resources`. For new | |
| // code, prefer to use fmt.Sprintf which enables more explicit and flexible formatting at the | |
| // potential cost of higher repetition. |
Nit: I lean towards deprecating this function entirely, in which case we should discouraged new usage, but that could be a separate follow-up discussion.
This PR moves the helper functions from provider.go to a new comparisons package. This package will be used by provider, tests, and resources. The refactoring will assist with splitting resources into resource specific packages. #106 The previous helper functions have been deprecated and will be removed in a future commit. Extracted from #622 (PR grew to large to maintain. Refactoring is not a priority and rebasing gets hard as the underlying code changes) Related to #674 Related to #654 Signed-off-by: Marques Johansson <[email protected]>
Extracted from #622
Also ran goimports for clean up (triggered by stray imports based test failure)