gohugoio / hugo

The world’s fastest framework for building websites.
https://gohugo.io
Apache License 2.0
75.38k stars 7.49k forks source link

Unexpected results with append template function #11131

Open jmooring opened 1 year ago

jmooring commented 1 year ago

Testing https://github.com/gohugoio/hugo/commit/12dc9a6e4acd5280a3e8b1658cbb96669fa97057:

PASS

{{ $s := slice | append "a" }}

[
  "a"
]

FAIL

{{ $s := slice | append (slice "a") }}

[
  "a"
]

PASS

{{ $s := slice | append (slice "a") (slice "b") }}

[
  [
    "a"
  ],
  [
    "b"
  ]
]

FAIL

{{ $s := slice | append (slice "a") }}
{{ $s = $s | append (slice "b") }}

[
  "a",
  "b"
]
bep commented 1 year ago

First, I don't see the relevance to this with https://github.com/gohugoio/hugo/commit/12dc9a6e4acd5280a3e8b1658cbb96669fa97057

{{ $s := slice | append (slice "a") }}
[
  "a"
]

I just checked, and the above was how it behaved also in v0.112.0 and it's not something we can change without breaking lots of sites, and I actually think the above behaviour is the most logical. It's not like we can add a dimension to the target slice.

{{ $s := slice | append (slice "a") (slice "b") }}

[
  [
    "a"
  ],
  [
    "b"
  ]
]

OK, the above doesn't look right, but it's not obvious what the right answer is. Again, I don't think we could/should expand the dimensions in the target slice. But again, this is how it behaved in older Hugo versions, so it's nothing new, right?

jmooring commented 1 year ago

I don't see the relevance to this with https://github.com/gohugoio/hugo/commit/12dc9a6e4acd5280a3e8b1658cbb96669fa97057

Sorry, what I meant was, I built Hugo at that commit for testing. But...

Let's set aside the above for a moment. I think I have/had a fundamental misunderstanding of how the append function is supposed to work. If I start with an empty slice, I thought I could add additional elements including scalars, slices, and maps. The root of my misunderstanding is that both of these produce the same result:

{{ $s := slice }}
{{ $s = $s | append 1 2 }}  # I want to append 2 scalar values, each as their own element

{{ $s := slice }}
{{ $s = $s | append (slice 1 2) }} # I want to append a slice of 2 scalar values as a single element

[
  1,
  2
]

The above is true for both v0.113.0 and v0.114.0. We didn't break anything, it just doesn't work the way I expected/wanted. And, as you pointed out, changing that behavior will break lots of sites.

So, to create a slice of slices (with both v0.113.0 and v0.114.0) I have to do this:

{{ $s := slice }}
{{ $s = $s | append (slice (slice 1 2)) }}

[
  [
    1,
    2
  ]
]

And while the syntax is a bit unexpected, I can certainly live with it. But we did break something going from v0.113.0 to v0.114.0.

v0.114.0

# example 1

{{ $s := slice }}
{{ $s = $s | append (slice (slice 1 2)) }}
{{ $s = $s | append (slice (slice 3 4)) }}

error calling append: cannot append slice of []int to slice of int

# example 2

{{ $s := slice }}
{{ $s = $s | append (slice (slice 1 2)) }}
{{ $s = $s | append (slice (slice "a" "b")) }}

error calling append: cannot append slice of []int to slice of string

# example 3

{{ $s := slice }}
{{ $s = $s | append (slice (slice 1 2)) }}
{{ $s = $s | append (slice (slice (dict "k" "v"))) }}

error calling append: cannot append slice of []map[string]interface {} to slice of int

v0.113.0

All three of the examples above worked as expected.

# example 1

[
  [
    1,
    2
  ],
  [
    3,
    4
  ]
]

# example 2

[
  [
    1,
    2
  ],
  [
    "a",
    "b"
  ]
]

# example 3

[
  [
    1,
    2
  ],
  [
    {
      "k": "v"
    }
  ]
]
jmooring commented 1 year ago

I've refactored the code I was using to build a data structure, creating a slice of maps instead of a slice of slices. So there's nothing urgent about this, at least for me. This is not a silent error, so I think we can put this on the back burner until it comes up again.

brennanangel commented 1 year ago

And while the syntax is a bit unexpected, I can certainly live with it. But we did break something going from v0.113.0 to v0.114.0.

@jmooring v0.114.0 broke our builds due to this issue. Thank you for highlighting it and offering an explanation and a workaround!

cmahnke commented 8 months ago

Since this came up as #11553: I also had the problem and fixed it in my base theme, so fixing this here will certainly break things again. A clean solution would be to change the behaviour back, at least for themes, when min_version < fixed version. But that would be a maintenance nightmare. Maybe we can agree that this change behaviour is the new default?