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

core: Add list() interpolation function #7528

Merged
merged 3 commits into from
Jul 20, 2016
Merged

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Jul 7, 2016

The list() interpolation function provides a way to add support for list literals to HIL without having to invent new syntax for it and modify the HIL parser.

It presents as a function, thus:

- list() -> []
- list("a") -> ["a"]
- list("a", "b") -> ["a", "b"]

Thanks to @wr0ngway for the idea of this approach, fixes #7460.

Assuming this is the route we wish to follow, we should make it work with types other than strings - the initial quick implementation is to get a feel for using a function rather than syntax.

cc @phinze, @mitchellh, @jbardin

@phinze
Copy link
Contributor

phinze commented Jul 7, 2016

Direction looks good. +1 for this as a stop-gap solution until we can parse non-primitives directly in HIL.

@jen20 jen20 force-pushed the f-list-interpolation-function branch from 989924a to 2c5e424 Compare July 8, 2016 10:20
@jen20
Copy link
Contributor Author

jen20 commented Jul 8, 2016

I've cleaned this up:

  • added a test to verify behaviour if a non-string is passed
  • removed the type checking code (which would never be hit) in favour of letting the type checker do its work
  • fixed formatting issues

I'll remove the WIP now - cc @phinze.

@jen20 jen20 changed the title [WIP] core: Add list() interpolation function core: Add list() interpolation function Jul 8, 2016
@phinze
Copy link
Contributor

phinze commented Jul 8, 2016

LGTM for strings. Agreed we should make it work for lists and maps as well.

@jen20 jen20 self-assigned this Jul 11, 2016
@phinze phinze self-assigned this Jul 11, 2016
jen20 and others added 2 commits July 18, 2016 18:12
The list() interpolation function provides a way to add support for list
literals (of strings) to HIL without having to invent new syntax for it
and modify the HIL parser.

It presents as a function, thus:

    - list() -> []
    - list("a") -> ["a"]
    - list("a", "b") -> ["a", "b"]

Thanks to @wr0ngway for the idea of this approach, fixes #7460.
Allow lists and maps within the list interpolation function via variable
interpolation. Since this requires setting the variadic type to TypeAny,
we check for non-heterogeneous lists in the callback.
@jbardin jbardin force-pushed the f-list-interpolation-function branch from 2c5e424 to 2bd7cfd Compare July 19, 2016 17:45
@jbardin
Copy link
Member

jbardin commented Jul 19, 2016

Expanded the list interpolation to accept lists and maps, and the concat function to accept lists of lists and lists of maps.

This now also fixes #7146

switch arg := arg.(type) {
case string:
// we also allow bare strings for backwards compatibility
outputList = append(outputList, ast.Variable{Type: ast.TypeString, Value: arg})
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's nuke the BC here. I believe we've already gone talking about this case like it's gone! 😀

@phinze
Copy link
Contributor

phinze commented Jul 19, 2016

Nice work @jbardin!! A few inline nits, but this looks super solid overall.

This will allow the concat interpolation function to accept lists of
lists, and lists of maps as well as strings. We still allow bare strings
for backwards compatibility, but remove some of the old comment wording
as it could cause confusion of this function with actual string
concatenation.

Since maps are now supported in the config, this removes the superfluous
(and failing) TestInterpolationFuncConcatListOfMaps.
@jbardin jbardin force-pushed the f-list-interpolation-function branch from 73d6158 to 8dcbc0b Compare July 19, 2016 21:45
@jbardin
Copy link
Member

jbardin commented Jul 19, 2016

Used Printable, since it will usually make a better user-facing error.
Squashed the minor changes.

@phinze
Copy link
Contributor

phinze commented Jul 20, 2016

LGTM

@jbardin jbardin merged commit d6d0c90 into master Jul 20, 2016
@jbardin jbardin deleted the f-list-interpolation-function branch July 20, 2016 21:04
@ghost
Copy link

ghost commented Apr 24, 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 Apr 24, 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.

Allow creating list in interpolations
3 participants