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

Fix unquoted list variables. #845

Closed
wants to merge 1 commit into from
Closed

Conversation

rgrinberg
Copy link
Member

This is the naive attempt to fix #699. It still needs some fixing to give proper locs in the case when the error occurs, but I'm not so certain we're taking the right approach here. I don't really like that expand and partial_expand have so much duplicate code and the way "multivalued" expansions are handled. I don't have suggestions how to improve this but I will try to improve the expansion code.

A single path cannot be obtained if the underlying expansion contains more than
1 path (via strings or paths)

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@ghost
Copy link

ghost commented Jun 2, 2018

I think to be more future proof regarding the other developments, we should implement exactly the semantic described in #701 (comment).

So technically, I think we can get rid of Concat_or_split and select the right behavior based on the context, which is one of:

  1. inside a quoted string
  2. as part of an atom but not a whole atom, such as x%{x}
  3. as a whole atom in a position where exactly one value is expected, such as (with-stdout-to %{x} (...))
  4. as a whole atom as argument of a variadic function, such as (run %{x})

In cases 2 and 3, it is an error if the variable doesn't expand to exactly 1 element. In other cases it may expand to any number of elements.

Additionally, in case 1 and 2, the value is always converted to a string, while in case 3 and 4 it retains it's type, i.e. string or path.

@rgrinberg
Copy link
Member Author

Closed in favor #849

@rgrinberg rgrinberg closed this Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant