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.

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

config: add formatlist #1829

Merged
merged 2 commits into from
May 13, 2015
Merged

config: add formatlist #1829

merged 2 commits into from
May 13, 2015

Conversation

josharian
Copy link
Contributor

With this PR, format now distributes over lists. Please see the commit comment and doc changes in f2fb9e7 for motivation and details.

@radeksimko
Copy link
Member

I like the idea and understand the motivation, but I feel that reusing the current format function is making that original function way too magical. I'd personally rather see a separate function handling this.

The reason is that I would never expect list to be output from a function that's called format.

@josharian
Copy link
Contributor Author

Would it help if format stayed as it was, and the newly-empowered function was called formatlist? formatlist could return an error if none of the params was a list.

FWIW, I experimented with a much less powerful version of this (josharian@ed79154), but concluded that it was helpful to be able to mix lists and scalars and to have multiple lists.

@josharian
Copy link
Contributor Author

Ok, I've moved list-oriented formatting to formatlist. Please take another look. (Also, let me know whether you'd like me to squash the history before merging. Leaving as-is for now because GitHub PRs don't handle rebasing gracefully, grrrr.)

@@ -11,8 +11,7 @@ import (
)

// InterpSplitDelim is the delimeter that is looked for to split when
// it is returned. This is a comma right now but should eventually become
// a value that a user is very unlikely to use (such as UUID).
// it is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Good spot.

@radeksimko
Copy link
Member

@josharian This looks good to me, so unless another maintainer jumps in here with a reason for not merging this, I will merge it.

The last thing I'd ask you to do before merging though is to squash your commits.

josharian added 2 commits May 12, 2015 16:11
formatlist distributes formatting over lists.
See the docs for details.

As a colleague commented:

"It happens all the time that we want a set of
outputs, but in a slightly different way than
just simple joining or concatting."

formatlist (combined with join)
makes it easy to satisfy those needs.
@josharian josharian changed the title Teach format to handle lists config: add formatlist May 12, 2015
@josharian
Copy link
Contributor Author

@radeksimko thanks. Squashed down to two commits and rebased.

}

if n == 0 {
return nil, errors.New("no lists in arguments to formatlist")
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't notice this before, but why exactly don't you use fmt.Errorf() here?

Copy link
Member

Choose a reason for hiding this comment

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

Stupid question... the answer is because you don't need any formatting 👐

radeksimko added a commit that referenced this pull request May 13, 2015
@radeksimko radeksimko merged commit 5d6eff0 into hashicorp:master May 13, 2015
@ghost
Copy link

ghost commented May 2, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants