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 substitutions in files included from within a list #312

Closed
wants to merge 8 commits into from

Conversation

rynonl
Copy link

@rynonl rynonl commented Apr 23, 2015

fixes #160

Allows for substitions/includes from within a list, and also allows += inside a list.

Regarding the test at ConcatenationTest:355 - This syntax is not specified as valid per the spec(at least not that I could tell) leading me to believe this is more like undefined behavior than related to the +=/includes within a list. I left the test in but with the exception that now gets thrown.

@havocp
Copy link
Collaborator

havocp commented Apr 23, 2015

Thanks for working on this!

I believe with this patch, if someone uses ${foo.0} in their config file, we would look up element 0 in the list foo, so we've added a new behavior that would need to be described in HOCON.md and also thought through in terms of implications. It may actually make sense - because we do let you treat a numerically-keyed object as an array, treating an array as a numerically-indexed object might be fine. But I hadn't thought of this solution before and so I need to think about it. It's important to "prove" to ourselves that nobody could be using the ${foo.0} to mean something else already in this case, so we know we won't break anyone with this.

@Test
def plusEqualsMultipleTimesNestedInPlusEquals() {
val e = intercept[ConfigException.Parse] {
val e = intercept[ConfigException.BugOrBroken] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want this to be invalid syntax, we should be throwing the Parse exception or a resolve-related exception, not the BugOrBroken.

I don't understand why it's invalid though; it expands to something like this right:

 x = ${?x} [ { a = ${?a} [1], a = ${?a} [2], a = ${?a} [3] } ]

It seems logical to me - the only "twist" here is that we have two nested lists, right? Or am I missing something?

The resolve code is definitely "fun" but it's probably possible to fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oops, I got the expansion wrong - should be something like a = ${?x.0.a} [1] etc. right?

@havocp havocp changed the title Substitutions in includes Fix substitutions in files included from within a list May 2, 2015
@rynonl
Copy link
Author

rynonl commented May 4, 2015

Added some information about indexing semantics for includes within lists to Hocon.md

As far as proving to ourselves that we won't break anyone...I can't think of a scenario where this would. If people are using ints as object keys now it should just continue chugging along because this patch explicitly looks for lists before appending indices to the path. You would know better than me about any edge cases though :)

I see what you mean regarding the += being valid syntax, however I think that is a separate issue than including from within lists. thoughts?

@rynonl rynonl closed this May 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Appending within an array could work if we could reference array elements in path expressions
2 participants