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.

Sign up for GitHub

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

chore: refactor nprintf #654

Merged
merged 2 commits into from
May 10, 2024
Merged

chore: refactor nprintf #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf81f3
chore: refactor nprintf to internal/nprintf
displague Apr 22, 2024
e144ee0
test: deprecate nprintf func used in NE tests
displague May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions equinix/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,11 @@ func (t *testAccConfig) build() string {
return t.config
}

// nprintf returns a string with all the placeholders replaced by the values from the params map
//
// Deprecated: nprintf is shared between NE resource tests and has been
// centralized ahead of those NE resources moving to separate packages.
// Use github.com/equinix/terraform-provider-equinix/internal/nprintf.NPrintf instead
func nprintf(format string, params map[string]interface{}) string {
for key, val := range params {
var strVal string
Expand Down
23 changes: 23 additions & 0 deletions internal/nprintf/nprintf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package nprintf

import (
"fmt"
"regexp"
"strings"
)

// NPrintf is a helper function to replace placeholders in a string with values from a map
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 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.

func NPrintf(format string, params map[string]interface{}) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

The nprintf extraction was made to simplify the changes and review needed for #622 where additional packages and functions were being moved out of equinix/ as part of #106 .

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@displague displague May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Duplicated and deprecated as separate commits. (removed: 6490d46 and e7708e9, the first may be cherry-picked later)

for key, val := range params {
var strVal string
switch val.(type) {
case []string:
r := regexp.MustCompile(`" "`)
strVal = r.ReplaceAllString(fmt.Sprintf("%q", val), `", "`)
default:
strVal = fmt.Sprintf("%v", val)
}
format = strings.Replace(format, "%{"+key+"}", strVal, -1)
}
return format
}
Loading